Skip to content
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

Conversation

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Oct 13, 2021

Q                       A
Fixed Issues? Workaround https://bugs.webkit.org/show_bug.cgi?id=220517
Patch: Bug Fix? Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

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 parameter a 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.

  • Tests on scope tracking changes.
  • Update compat-table after kangax/compat-table#1765 is merged
  • Add bugfix plugin to preset-env

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Oct 13, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49271/

@codesandbox
Copy link

@codesandbox codesandbox bot commented Oct 13, 2021

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:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from cf99914 to 947c7e1 Oct 13, 2021
const { scope } = path;
// invariant: path.node.id is always an Identifier here
const newParamName = scope.generateUid(name);
scope.rename(name, newParamName);
Copy link
Contributor Author

@JLHwung JLHwung Oct 13, 2021

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.

@JLHwung JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 947c7e1 to 3b01e94 Oct 13, 2021
@JLHwung JLHwung changed the title Implement @babel/plugin-bugfix-webkit-id-destructuring-collision-in-function-expression Implement @babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression Oct 13, 2021
@JLHwung JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 2779bf1 to b9b5ae7 Oct 13, 2021
@JLHwung JLHwung marked this pull request as ready for review Oct 13, 2021
allReplacedFeatures[replaces] = [];
}
generatedTargets[plugin] = {};
for (const replace of replaces) {
Copy link
Contributor Author

@JLHwung JLHwung Oct 13, 2021

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.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 20, 2021

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.

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"
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 15, 2021

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.

Copy link
Contributor Author

@JLHwung JLHwung Oct 15, 2021

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.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 17, 2021

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:

  1. Leave a lower peerDependency
  2. Change the api.assertVersion call in the new plugin to api.assertVersion("7.16.0"), so that it doesn't fail silently.
  3. Add this plugin to the list at
    export const minVersions = {
    , so that it's only enabled when using new @babel/core versions.

},
"bugfix/transform-safari-id-destructuring-collision-in-function-expression": {
features: ["destructuring, parameters / duplicate identifier"],
replaces: ["transform-parameters", "proposal-object-rest-spread"],
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 15, 2021

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?

Copy link
Contributor Author

@JLHwung JLHwung Oct 15, 2021

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.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 17, 2021

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 😅

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

I'm surprised; I would have expected transform-parameters's Safari version in babel-compat-data/data/plugin.json to change 🤔

@JLHwung
Copy link
Contributor Author

@JLHwung JLHwung commented Oct 17, 2021

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 kangax/compat-table#1767

@JLHwung JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 19e22e4 to f6cad37 Oct 17, 2021
@JLHwung JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from f6cad37 to 40748d2 Oct 19, 2021
@JLHwung JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 141bd07 to f8a2830 Oct 19, 2021
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Awesome work!

allReplacedFeatures[replaces] = [];
}
generatedTargets[plugin] = {};
for (const replace of replaces) {
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 20, 2021

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;
Copy link
Contributor Author

@JLHwung JLHwung Oct 20, 2021

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.

@JLHwung JLHwung merged commit 29f697c into babel:main Oct 20, 2021
26 of 28 checks passed
@JLHwung JLHwung deleted the bugfix-safari-id-destructuring-collision-in-function-expression branch Oct 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants