Introduce bugfix-safari-class-field-initializer-scope
#16569
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/57460 |
|
@davidtaylorhq I'm starting to prepare the 7.25.0 release -- do you have any idea for when you'll have time to finish this PR? :) |
|
Thanks for the note @nicolo-ribaudo - I'll see if I can find some time this week. What's the timeline on 7.25.0? |
|
I'm targeting mid/end of next week. If needed, I can also help with this PR, but it seems like it's already almost complete. |
nicolo-ribaudo
added
PR: New Feature 🚀
| ) { | ||
| const { value } = path.node; | ||
|
|
||
| if (value && !(t.isLiteral(value) && !t.isTemplateLiteral(value))) { |
There was a problem hiding this comment.
The condition for when wrapping is necessary is if there is anything wrapped in parentheses that is outside of a function, right?
To avoid too many IIFEs, I would expand this logic to, recursively:
- if NODE is a non-template literal, or a Function, return
false - if NODE is a CallExpression or a NewExpression, return
trueif recursing on the callee or on any of the arguments returnstrue - if NODE is a TemplateLiteral, return
trueif recursing on the callee or on any of the inner expressions returnstrue - if NODE is a TaggedTemplateExpression, return
trueif recursing on the tag or on the template returnstrue - if NODE is an ArrayExpression, return
trueif recursing on any of the elements returnstrue - if NODE is an ObjectExpression, return
trueif recursing on any of the property values or computed keys returnstrue - if NODE is a MemberExpression, return true if the object or the computed property returns
true - if NODE is an Identifier, return
false
This should avoid parens in most cases, except for when they are strictly necessary
There was a problem hiding this comment.
The condition for when wrapping is necessary is if there is anything wrapped in parentheses that is outside of a function, right?
Not quite. The presence of parenthesis anywhere in the expression is enough to trigger the bug. The check added in compat-table demonstrates this. c = a[0] is fine. c = a[(0)] is not 😬
In this comment of #14289, concern was raised about doing any transformation based on the presence (or not) of parenthesis. It's quite hard to do reliably.
That said, we can certainly cut down on the number of initializers which need to be transformed. Instead of gating it on parenthesis, we can look for any Identifiers which may refer to the parent scope. In fact, the logic we need is almost identical to what you wrote out, except that the last line should be:
- If NODE is an identifier, return
falsetrue
I've also added handling for a few other 'safe' nodes which can be skipped:
- added: "if NODE is a FunctionExpression or ArrowFunctionExpression, return
false" - added: "if NODE is a ThisExpression, return
false" - added: "if NODE is a SequenceExpression, return
trueif any of the elements returnstrue"
davidtaylorhq
force-pushed
the
safari-class-field-initializer-scope
branch
2 times, most recently
from
90e9011
to
eef8c82
Compare
See babel/babel#16569 Doc structure adapted from `plugin-bugfix-firefox-class-in-computed-class-key`
|
@nicolo-ribaudo this is now in a much better state - thanks for the help. Please let me know if you have any comments/suggestions. Is there anything we need to do to prepare for the new Also, is there any good way to test a development version of |
davidtaylorhq
force-pushed
the
safari-class-field-initializer-scope
branch
from
eef8c82
to
70abe65
Compare
See babel/babel#16569 Doc structure adapted from `plugin-bugfix-firefox-class-in-computed-class-key`
davidtaylorhq
force-pushed
the
safari-class-field-initializer-scope
branch
from
70abe65
to
5827ad4
Compare
| return false; | ||
| } | ||
|
|
||
| if (t.isCallExpression(node) || t.isNewExpression(node)) { |
There was a problem hiding this comment.
We should also cover the OptionalCallExpression.
| }); | ||
| } | ||
|
|
||
| if (t.isMemberExpression(node)) { |
There was a problem hiding this comment.
And the OptionalMemberExpression.
| ); | ||
| } | ||
|
|
||
| if (t.isFunctionExpression(node) || t.isArrowFunctionExpression(node)) { |
There was a problem hiding this comment.
Should we also cover ClassExpression here?
There was a problem hiding this comment.
Yup, added 👍
I have confirmed that the superClass expression does not seem to be affected by the Safari bug, so we don't need to recurse into that.
davidtaylorhq
force-pushed
the
safari-class-field-initializer-scope
branch
from
5827ad4
to
0072adf
Compare
This tightens things up to reduce the number of initializers which need to be wrapped in an IIFE. Mirrors the changes made in babel/babel#16569
This tightens things up to reduce the number of initializers which need to be wrapped in an IIFE. Mirrors the changes made in babel/babel#16569
nicolo-ribaudo
added
the
PR: Ready to be Merged
Ideally we'd need a basic docs PR, to add a file like https://github.com/babel/website/blob/main/docs/plugin-bugfix-firefox-class-in-computed-class-key.md |
|
Here's the docs PR: babel/website#2921 |
github-actions
bot
added
the
outdated
This is a workaround for https://bugs.webkit.org/show_bug.cgi?id=236843, and aims to resolve #14289.
It adds the new compat-table check to the requirements for
transform-class-properties, which means that transform will now apply for Safari < 16 (previously Safari < 14.5).It also introduces a bugfix transformation which can be used as an alternative to transform-class-properties on Safari 15. This bugfix transformations wraps potentially-affected initializers in an IIFE. A number of shortcuts are added to avoid transforming initializers which are guaranteed to be unaffected by the bug.
Todo: