-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Account for template literals revision #5523
Conversation
Codecov Report
@@ Coverage Diff @@
## 7.0 #5523 +/- ##
==========================================
+ Coverage 85.38% 85.39% +<.01%
==========================================
Files 200 200
Lines 9492 9495 +3
Branches 2696 2698 +2
==========================================
+ Hits 8105 8108 +3
Misses 889 889
Partials 498 498
Continue to review full report at Codecov.
|
_templateObject6 = _taggedTemplateLiteral(["left", "right"], ["left", "\\u000g", "right"]), | ||
_templateObject7 = _taggedTemplateLiteral(["left", "right"], ["left", "\\u{-0}", "right"]); | ||
|
||
function _taggedTemplateLiteral(strings, raw) { return Object.freeze(Object.defineProperties(strings, { raw: { value: Object.freeze(raw) } })); } |
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 actually didn't know this required defineProperty :o
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 didn't either - maybe this..
right this is the non-loose mode one.
function _taggedTemplateLiteralLoose(strings, raw) { strings.raw = raw; return strings; }
@@ -18,7 +18,9 @@ export default function ({ types: t }) { | |||
let raw = []; | |||
|
|||
for (const elem of (quasi.quasis: Array)) { | |||
strings.push(t.stringLiteral(elem.value.cooked)); | |||
if (elem.value.cooked !== null) { |
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.
Should this be left out, or explicitly be undefined
/void 0
in the array?
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.
Not sure what's supposed to happen for .length
of strs
or if there is danger that one of several pieces might be missing cooked, like what is the output for \unicode${45}\u0065
, are both pieces missing the .cooked
?
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.
It should be present with value undefined. Length is still the same as that of .raw.
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.
Cool. Is every cooked value excluded if one is, or is it each individual value either may or may not use invalid escapes independent of the others?
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.
They're independent. (That's how Babylon constructs the AST, too.)
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.
Cool, wanted to verify my understanding then. So @hzoo this should do
const value = elem.value.cooked == null
? t.unaryExpression("void", t.numericLiteral(0))
: t.stringLiteral(elem.value.cooked);
strings.push(value);
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.
what about just t.identifier("undefined")
? turns into void 0
already I thought
-var _templateObject = _taggedTemplateLiteral([undefined], ["\\unicode and \\u{55}"]),
- _templateObject2 = _taggedTemplateLiteral([undefined], ["\\01"]),
- _templateObject3 = _taggedTemplateLiteral([undefined, "right"], ["\\xg", "right"]),
- _templateObject4 = _taggedTemplateLiteral(["left", undefined], ["left", "\\xg"]),
- _templateObject5 = _taggedTemplateLiteral(["left", undefined, "right"], ["left", "\\xg", "right"]),
- _templateObject6 = _taggedTemplateLiteral(["left", undefined, "right"], ["left", "\\u000g", "right"]),
- _templateObject7 = _taggedTemplateLiteral(["left", undefined, "right"], ["left", "\\u{-0}", "right"]);
+var _templateObject = _taggedTemplateLiteral([], ["\\unicode and \\u{55}"]),
+ _templateObject2 = _taggedTemplateLiteral([], ["\\01"]),
+ _templateObject3 = _taggedTemplateLiteral(["right"], ["\\xg", "right"]),
+ _templateObject4 = _taggedTemplateLiteral(["left"], ["left", "\\xg"]),
+ _templateObject5 = _taggedTemplateLiteral(["left", "right"], ["left", "\\xg", "right"]),
+ _templateObject6 = _taggedTemplateLiteral(["left", "right"], ["left", "\\u000g", "right"]),
+ _templateObject7 = _taggedTemplateLiteral(["left", "right"], ["left", "\\u{-0}", "right"]);
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.
k plz approve or thumb if ready 🙌
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.
@hzoo var undefined = 0
works at least in sloppy mode, so you do need `void 0 or equivalent. Otherwise lgtm.
@@ -18,7 +18,10 @@ export default function ({ types: t }) { | |||
let raw = []; | |||
|
|||
for (const elem of (quasi.quasis: Array)) { | |||
strings.push(t.stringLiteral(elem.value.cooked)); | |||
const value = elem.value.cooked == null | |||
? t.identifier("undefined") |
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.
There could be a var undefined = 4
in the top scope of the module so it would break. You could also use path.scope.buildUndefinedNode()
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.
yep sorry forgot the name of the method
eeb3e8f
to
b7aa909
Compare
changed to use also added tests for spec/loose mode (no differences really) |
btw @bakkot do you want to be a collaborator? (messaged you on slack) |
Any chance this could be backported to v6? |
Yes this is a bug fix but also a breaking changes for people relying on this (don't want to know why 😄), it also require Babylon to backport it. I don't think we want to land this on 6. |
Relying on a tagged template literal throwing? |
I think this+babylon seems backportable. Probably a good issue to file as a |
As discussed on Slack, i'm fine landing this in 6. |
Input
Output
Fixes #4798
cc @bakkot @TimothyGu