-
Notifications
You must be signed in to change notification settings - Fork 470
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
Add JSON ⊂ ECMAScript tests #1543
The head ref may contain hidden characters: "json-\u2282-ecmascript"
Conversation
8e5095c
to
8145c86
Compare
cc @gibson042 |
features: [json-superset] | ||
---*/ | ||
|
||
eval("'foo\u2028bar\u2028baz\u2028qux'"); |
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.
this should go a bit beyond here. We don't need eval for these new tests, that's only legacy we don't need to import from the removed file.
At the same time, it's also a good opportunity to duplicate these tests with the literal cases using unescaped and escaped forms in each test file. I can work in a follow up if it's such a burn to add more here, let me know and I'd be glad to help.
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.
I used eval
to avoid having to put the raw U+2028 and U+2029 characters in the source code, which is hard to review and would be easy to accidentally break in the future. This approach seemed more maintainable.
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.
eval
is annoying because it makes these tests not really consumable by parsers and other consumers which don't implement eval
. (See #1356 etc.) I think it's worth having the literal character in the source.
features.txt
Outdated
@@ -77,6 +77,10 @@ numeric-separator-literal | |||
String.prototype.matchAll | |||
Symbol.matchAll | |||
|
|||
# JSON ⊂ ECMAScript |
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.
This is probably better written as "ECMAScript ⊃ JSON" given the proposal name.
8145c86
to
76843c3
Compare
Thanks for the feedback. Patch updated. |
Oops, I opened #1544 without seeing this. My mistake. |
I like the approach in #1544. Let’s go with that PR instead. |
Proposal: https://github.com/tc39/proposal-json-superset