-
-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(functions): schema fn should allow redundant escapes #1787
Conversation
strict: false, | ||
allowUnionTypes: true, | ||
logger, | ||
unicodeRegExp: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we want to remove Unicode-related features? This definitely fixes the issue, but introduces some limitation to the shape of regex (according to MDN)
WDYT about somehow passing String.raw
$stringliteral`` instead of string
when we create `new RegExp()`? (although this might require pushing changes to `Ajv` itself)
Or just changing \\
to '\\\\
whenever we see \\
in a string literal pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about somehow passing String.raw$stringliteral`` instead of string when we create
new RegExp()
? (although this might require pushing changes to `Ajv` itself)
Or just changing \ to '\\ whenever we see \ in a string literal pattern?
That won't fix the issue, as this changes the semantics of the regular expression.
Replacing\\
with \\\\
leads to a different regular expression.
For instance
new RegExp('[\\_-]').test('_'); // true
new RegExp('[\\_-]').test('\\'); // false
new RegExp('[\\\\_-]').test('\\'); // true
In a nutshell, the problem here is that when a u
(unicode) flag is supplied, the behavior of \
changes a bit, and it's up to a end user to account for that.
Unfortunately, this means that the only choices we're left here include
a) tell users to write more proper regular expressions
b) get rid of redundant escapes - very tricky do to right
c) switch to non-unicode regexps
In other words, there's no good solution, each of them has some trade-offs.
Luckily, I haven't really seen much usage of unicode, the vast majority of users doesn't use it at all. The the spec itself does not enforce the unicode support, it recommends it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip you convinced me. We'll always have to pick lesser evil
# [@stoplight/spectral-functions-v1.2.0](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-functions-v1.1.2...@stoplight/spectral-functions-v1.2.0) (2021-09-01) ### Bug Fixes * **functions:** schema fn should allow redundant escapes ([#1787](#1787)) ([5a29d44](5a29d44)) ### Features * **functions:** output Ajv runtime exceptions ([#1801](#1801)) ([5a19625](5a19625))
🎉 This PR is included in version @stoplight/spectral-functions-v1.2.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Fixes #1782
Checklist
Does this PR introduce a breaking change?
Additional context
This is not a bug in Ajv or anything, it's just how the unicode flag works.