Skip to content

Fix _objectSpread2 for real#10188

Merged
nicolo-ribaudo merged 3 commits into
babel:masterfrom
nicolo-ribaudo:lololol
Jul 9, 2019
Merged

Fix _objectSpread2 for real#10188
nicolo-ribaudo merged 3 commits into
babel:masterfrom
nicolo-ribaudo:lololol

Conversation

@nicolo-ribaudo

@nicolo-ribaudo nicolo-ribaudo commented Jul 9, 2019

Copy link
Copy Markdown
Member
Q                       A
Fixed Issues? Fixes #10166 for real maybe I hope
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

I can't use arguments from an outer function inside an inner function.
I can't use arguments from an outer function inside an inner function.
I can't use arguments from an outer function inside an inner function.
I can't use arguments from an outer function inside an inner function.

(Thanks @hzoo for helping me finding the bug)

@maxmodel or @pwnreza Could you try to update the code in @babel/helpers inside your node_modules like I did in this PR and confirm that it works?

I'll now go hiding myself somewhere and I won't touch an helper for a few months 😆

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Object Rest/Spread area: helpers labels Jul 9, 2019
target,
key,
Object.getOwnPropertyDescriptor(arguments[i], key)
Object.getOwnPropertyDescriptor(source, key)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was the bug 🪲 🪲🪲🪲🪲🪲

@hzoo hzoo Jul 9, 2019

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.

Right! the forEach on line 433 (ownKeys(source).forEach(function (key) {) is it's own function scope so the arguments[i] usage here is the issue. Solution is to save it as a variable up top and reuse it everywhere.

Object.getOwnPropertyDescriptors = null;

({ ...{ a: 1 }, ...{} });
({ ...{ a: 1 }, b: 1, ...{} });

@nicolo-ribaudo nicolo-ribaudo Jul 9, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This updated test failed before the fix.

@hzoo hzoo left a comment

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.

👍 thanks, and it's totally cool, we all make mistakes and it's a good learning opportunity! (yes it's unfortunate it happens to be on something a lot of people use but you've been doing such a great job)

@babel-bot

Copy link
Copy Markdown
Collaborator

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

@maxmodel

maxmodel commented Jul 9, 2019

Copy link
Copy Markdown

Confirmed that this fixes the problem -- thanks so much @nicolo-ribaudo!

@nicolo-ribaudo

Copy link
Copy Markdown
Member Author

Also confired by @belemaire #10166 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: helpers outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Object Rest/Spread

Projects

None yet

Development

Successfully merging this pull request may close these issues.

undefined is not a function(evaluation 'object.getOwnPropertyDescriptors(arguments[i])')

5 participants