Skip to content

Add new test for destructuring param with id collision#1765

Merged
chicoxyzzy merged 3 commits intocompat-table:gh-pagesfrom
jridgewell:duplicate-destructure-identifier
Oct 13, 2021
Merged

Add new test for destructuring param with id collision#1765
chicoxyzzy merged 3 commits intocompat-table:gh-pagesfrom
jridgewell:duplicate-destructure-identifier

Conversation

@jridgewell
Copy link
Copy Markdown
Contributor

Safari 15 has issue with destructuring parameters when a function expression's id collides with a destructured parameter identifier.

https://bugs.webkit.org/show_bug.cgi?id=220517

Safari 15 has issue with destructuring parameters when a function expression's id collides with a destructured parameter identifier.

https://bugs.webkit.org/show_bug.cgi?id=220517
Comment on lines +17301 to +17306
try {
eval('var f = function f([id, id]) { return id }');
return false;
} catch (e) {
return e instanceof SyntaxError;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we could make this test pass when destructuring isn't supported? that way the only false values will be those that implemented destructuring incorrectly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can wrap everything in evals, add a check for basic destructuring, and early exit.

But, I think what's really needed is a "bug" check on top of another test. Failures would be listed, and absence would imply that it's supported (or explicitly listed as supported, as I imagine 15.1 will need).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, that sounds like a larger refactor.

I think for now this is fine, but it feels like the false results for engines that don't have destructuring in the first place (like opera) are a bit useless :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the extra false results.

@ljharb ljharb requested a review from chicoxyzzy October 9, 2021 01:54
data-es6.js Outdated
Comment on lines +17298 to +17299
var d = function d([d]) { return d };
if (!d([true])) return false;
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var d = function d([d]) { return d };
if (!d([true])) return false;
if(!(function d([d]) {})) return false;

Golf down the reproducing example a bit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually checking that it returned the not-function binding, but I see I forgot the check explicitly for !== true.

@chicoxyzzy chicoxyzzy merged commit 1ac7f90 into compat-table:gh-pages Oct 13, 2021
@jridgewell jridgewell deleted the duplicate-destructure-identifier branch October 13, 2021 16:23
JLHwung pushed a commit to JLHwung/compat-table that referenced this pull request Mar 30, 2023
…1765)

* Add new test for destructuring param with id collision

Safari 15 has issue with destructuring parameters when a function expression's id collides with a destructured parameter identifier.

https://bugs.webkit.org/show_bug.cgi?id=220517

* Remove false values

* Fix test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants