Implement @babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression by JLHwung · Pull Request #13842 · babel/babel

JLHwung · 2021-10-13T00:17:19Z

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 Add new test for destructuring param with id collision compat-table/compat-table#1765 is merged
Add bugfix plugin to preset-env

babel-bot · 2021-10-13T00:19:50Z

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

codesandbox-ci · 2021-10-13T00:20:35Z

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 · 2021-10-13T15:37:27Z

...es/babel-plugin-bugfix-webkit-id-destructuring-collision-in-function-expression/src/index.ts

+          const { scope } = path;
+          // invariant: path.node.id is always an Identifier here
+          const newParamName = scope.generateUid(name);
+          scope.rename(name, newParamName);


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 · 2021-10-13T19:32:35Z

packages/babel-compat-data/scripts/build-bugfixes-targets.js

-    allReplacedFeatures[replaces] = [];
-  }
-  generatedTargets[plugin] = {};
+  for (const replace of replaces) {


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.

This can be reverted, since we don't need it anymore.

packages/babel-plugin-bugfix-safari-id-destructuring-collision-in-function-expression/README.md

nicolo-ribaudo · 2021-10-15T18:17:18Z

Makefile

+	@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"


This is a breaking change, since we would have to also bump the peerDependencies version in @babel/preset-env.

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.

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 to api.assertVersion("7.16.0"), so that it doesn't fail silently.

Add this plugin to the list at

babel/packages/babel-preset-env/src/available-plugins.ts

Line 149 in 513b00e

export const minVersions = {

, so that it's only enabled when using new @babel/core versions.

nicolo-ribaudo · 2021-10-15T18:18:02Z

packages/babel-compat-data/scripts/data/plugin-bugfixes.js

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


Why do we also need to transform proposal-object-rest-spread in safari?

(function a({ ...a }) {});

also throws on Safari. See https://github.com/babel/babel/pull/13842/files#diff-6a0282b86f9119d73cd3eefe8f38ce1d4f94562a58c7c47bd75ffb124d2e432dR23 for more affected cases.

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 😅

nicolo-ribaudo

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

JLHwung · 2021-10-17T19:33:42Z

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

…-in-function-expression/README.md

nicolo-ribaudo

Awesome work!

nicolo-ribaudo · 2021-10-20T07:59:32Z

packages/babel-compat-data/scripts/build-bugfixes-targets.js

-    allReplacedFeatures[replaces] = [];
-  }
-  generatedTargets[plugin] = {};
+  for (const replace of replaces) {


This can be reverted, since we don't need it anymore.

JLHwung · 2021-10-20T13:51:53Z

...abel-plugin-transform-react-constant-elements/test/fixtures/integration/issue-7610/output.js


+  let {
+    closeFn
+  } = _ref;


The test is for Babel 8 only. We should work on enabling bugfixes by default in Babel 8.

JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories i: browser bug labels Oct 13, 2021

JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from cf99914 to 947c7e1 Compare October 13, 2021 15:33

JLHwung commented Oct 13, 2021

View reviewed changes

JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 947c7e1 to 3b01e94 Compare October 13, 2021 18:52

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 force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 2779bf1 to b9b5ae7 Compare October 13, 2021 19:13

JLHwung marked this pull request as ready for review October 13, 2021 19:13

JLHwung commented Oct 13, 2021

View reviewed changes

packages/babel-plugin-bugfix-safari-id-destructuring-collision-in-function-expression/README.md Outdated Show resolved Hide resolved

jridgewell approved these changes Oct 13, 2021

View reviewed changes

nicolo-ribaudo reviewed Oct 15, 2021

View reviewed changes

nicolo-ribaudo reviewed Oct 17, 2021

View reviewed changes

JLHwung added the ⚠️ Do not merge label Oct 17, 2021

JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 19e22e4 to f6cad37 Compare October 17, 2021 23:06

JLHwung added 11 commits October 19, 2021 10:16

fix: register function expression id after params

4368822

implement bugfix plugin

51c00f2

add more testcases

d78f3fd

fix: do not skip pattern binding referencing id

0e31517

update compat-table

6d8e8c1

add bugfix plugin to preset-env

b53a29a

update Babel 8 test fixtures

5632a4d

Update packages/babel-plugin-bugfix-safari-id-destructuring-collision…

e354db3

…-in-function-expression/README.md

chore: bundle bugfix plugin

4e90f11

address review comments

2aa123e

add runtime version check

6d7bcd9

update compat table

40748d2

JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from f6cad37 to 40748d2 Compare October 19, 2021 14:17

JLHwung removed the ⚠️ Do not merge label Oct 19, 2021

JLHwung added 2 commits October 19, 2021 10:19

fix syntax error

faa978d

update test fixtures

f8a2830

JLHwung force-pushed the bugfix-safari-id-destructuring-collision-in-function-expression branch from 141bd07 to f8a2830 Compare October 19, 2021 18:48

nicolo-ribaudo approved these changes Oct 20, 2021

View reviewed changes

JLHwung added 2 commits October 20, 2021 09:46

revert bugfixes targets update

28ce449

update Babel 8 test fixtures

c372660

JLHwung commented Oct 20, 2021

View reviewed changes

JLHwung merged commit 29f697c into babel:main Oct 20, 2021

JLHwung deleted the bugfix-safari-id-destructuring-collision-in-function-expression branch October 20, 2021 19:53

JLHwung mentioned this pull request Nov 1, 2021

skip id-destructuring bugfix when binding info is not found #13910

Merged

snyk-bot mentioned this pull request Dec 14, 2021

[Snyk] Upgrade @babel/core from 7.9.6 to 7.16.0 aml-org/playground#69

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 20, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2022

Uh oh!

Conversation

JLHwung commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

JLHwung commented Oct 17, 2021

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JLHwung commented Oct 13, 2021 •

edited

Loading

babel-bot commented Oct 13, 2021 •

edited

Loading

codesandbox-ci bot commented Oct 13, 2021 •

edited

Loading

nicolo-ribaudo Oct 17, 2021 •

edited

Loading