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
Implement @babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression #13842
Implement @babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression #13842
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49271/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c372660:
|
cf99914
to
947c7e1
Compare
const { scope } = path; | ||
// invariant: path.node.id is always an Identifier here | ||
const newParamName = scope.generateUid(name); | ||
scope.rename(name, newParamName); |
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.
Related: Since the function id is registered as a constantViolation, the scope.rename
can't rename the function id here, maybe we need scope.rename(NodePath, string)
interface.
947c7e1
to
3b01e94
Compare
2779bf1
to
b9b5ae7
Compare
allReplacedFeatures[replaces] = []; | ||
} | ||
generatedTargets[plugin] = {}; | ||
for (const replace of replaces) { |
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.
The script is updated to support multiple replaces
config. e.g. bugfix/transform-safari-id-destructuring-collision-in-function-expression
replaces both transform-parameters
and proposal-object-rest-spread
.
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 can be reverted, since we don't need it anymore.
packages/babel-plugin-bugfix-safari-id-destructuring-collision-in-function-expression/README.md
Outdated
Show resolved
Hide resolved
Makefile
Outdated
@echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!" | ||
@echo "!!!!!! !!!!!!" | ||
@echo "!!!!!! Set packages/babel-plugin-bugfix-safari-id-destructuring-collision-in-function-expression/package.json" | ||
@echo "!!!!!! @babel/core peerDependencies to latest published version" |
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 a breaking change, since we would have to also bump the peerDependencies
version in @babel/preset-env
.
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.
Well proposal-static-block
depends on @babel/core@7.12.0
and it seems ... people are fine about that? I can remove the peer deps but the bugfix plugin depends on a bugfix of @babel/traverse
so it's better to specify the peer deps than failing silently.
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.
Well, when we added proposal-static-block
to @babel/preset-env@7.12.0
was already six months old, so it was likely that many people already updated it. This time we are forcing everyone to update @babel/core
.
I think that it would be better to:
- Leave a lower
peerDependency
- Change the
api.assertVersion
call in the new plugin toapi.assertVersion("7.16.0")
, so that it doesn't fail silently. - Add this plugin to the list at
export const minVersions = { @babel/core
versions.
}, | ||
"bugfix/transform-safari-id-destructuring-collision-in-function-expression": { | ||
features: ["destructuring, parameters / duplicate identifier"], | ||
replaces: ["transform-parameters", "proposal-object-rest-spread"], |
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.
Why do we also need to transform proposal-object-rest-spread
in safari?
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.
(function a({ ...a }) {});
also throws on Safari. See https://github.com/babel/babel/pull/13842/files#diff-6a0282b86f9119d73cd3eefe8f38ce1d4f94562a58c7c47bd75ffb124d2e432dR23 for more affected cases.
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.
Even with just transform-parameters
that code is transformed to
(function a(_ref) {
let { ...a
} = _ref;
return function () {}();
});
which should work in Safari?
Since many people don't enable bugfixes
, I'm trying to minimize the default generated output 😅
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'm surprised; I would have expected transform-parameters
's Safari version in babel-compat-data/data/plugin.json
to change 🤔
Good catch! It will be fixed in compat-table/compat-table#1767 |
19e22e4
to
f6cad37
Compare
…-in-function-expression/README.md
f6cad37
to
40748d2
Compare
141bd07
to
f8a2830
Compare
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.
Awesome work!
allReplacedFeatures[replaces] = []; | ||
} | ||
generatedTargets[plugin] = {}; | ||
for (const replace of replaces) { |
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 can be reverted, since we don't need it anymore.
var _Child; | ||
|
||
let { | ||
closeFn | ||
} = _ref; |
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.
The test is for Babel 8 only. We should work on enabling bugfixes
by default in Babel 8.
Thanks to @jridgewell who brings up this issue.
In this PR we introduce a new bugfix plugin which detects the affected pattern
(function a({ a }) {})
or(function a(...a) {})
and rename the parametera
to an unique variable that does not collide with other parameters.The plugin relies on the binding information determined by the babel traverse scope tracking. More bugs are found and fixed during development with new tests.
compat-table
after Add new test for destructuring param with id collision compat-table/compat-table#1765 is mergedpreset-env