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

Fix destructuring assignments being transpiled for edge 15 #9902

Merged
merged 2 commits into from May 3, 2019

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Apr 26, 2019

Q                       A
Fixed Issues? No existing issue found
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

preset-env transpiles object destructuring in edge 17.

Current behavior: 2254f7c#diff-0d3abae52af132aba0fb19ce6f915ccf
Expected behavior: 24a0e71#diff-0d3abae52af132aba0fb19ce6f915ccf

The destructuring assignment is implemented as early as edge 15 (loosely in 14) though. Was this required for some transform dependencies when function parameters are transpiled? Specifically destructuring in the function parameters is only properly implemented in later browsers.

Will need to check if those are still transpiled properly in the deploy preview in this PR.

On another note: preset-env is really nice but I wish the documentation would be more clear that there are legitimate use cases for specifiyng each transform instead of using the preset. transform-destructuring is currently used up to firefox 53 just because the return in custom iterator implementations is not called. I don't think this has any impact on basic object destructuring which would allow me to drop the transform for firefox 49. Maybe this is more of an issue with the mdn page which lists object destructuring as being implemented in firefox 49 but then babel transpiles it anyway. Pretty confusing but I guess this is what browser support still requires.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Apr 26, 2019

Can you add a test for this code?

((a, {b = 0, c = 3}) => {
  return a === 1 && b === 2 && c === 3;
})(1, {b: 2});

It's the code for which compat-table reports edge 15 as unsupported.


MDN isn't really reliable, I suggest using https://kangax.github.io/compat-table/es6/ instead.

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 27, 2019

MDN isn't really reliable, I suggest using kangax.github.io/compat-table/es6 instead.

For sure. But the outcome was the same. preset-env behavior didn't match what I would expect from the compat table.

Can you add a test for this code?

((a, {b = 0, c = 3}) => {
  return a === 1 && b === 2 && c === 3;
})(1, {b: 2});

It's the code for which compat-table reports edge 15 as unsupported.

I would expect only transform-parameters to be applied in that case which results in

((a, _ref) => {
  let { b = 0, c = 3 } = _ref;
  return a === 1 && b === 2 && c === 3;
})(1, {
  b: 2
});

which is edge 15 compatible.

Didn't quite click for me where to put the test but I got it now. Reworking the PR.

@nicolo-ribaudo
Copy link
Member

Yeah, the parameters transform is enough

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 27, 2019

24a0e71 should make my intentions clearer. Hope this is the right place for the test.

Got one test failing locally but this happened before any changes. Maybe a rebase will fix it.

@nicolo-ribaudo
Copy link
Member

Yeah, it's fixed on master. If the only failing test is that one, it's ok.

@babel-bot
Copy link
Collaborator

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

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 27, 2019

Looks good.

@nicolo-ribaudo nicolo-ribaudo merged commit eae7a33 into babel:master May 3, 2019
@eps1lon eps1lon deleted the fix/preset-env-obj-destr branch May 21, 2019 19:08
eps1lon added a commit to eps1lon/babel that referenced this pull request May 21, 2019
eps1lon added a commit to eps1lon/babel that referenced this pull request May 21, 2019
nicolo-ribaudo pushed a commit that referenced this pull request May 21, 2019
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants