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 objectSpread2 backward compatibility #10170

Merged
merged 1 commit into from Jul 6, 2019

Conversation

@nicolo-ribaudo
Copy link
Member

commented Jul 5, 2019

Q                       A
Fixed Issues? Fixes #10169
Patch: Bug Fix? yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT
@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

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

1 similar comment
@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

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

@nicolo-ribaudo nicolo-ribaudo merged commit 24dde2e into babel:master Jul 6, 2019

5 checks passed

babel/repl REPL preview is available
Details
buildsize No prior size to compare - 8.01 MB
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.55% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:object-rest-spread-2 branch Jul 6, 2019

@XhmikosR

This comment has been minimized.

Copy link

commented Jul 6, 2019

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

It seems like there is a problem with the way rollup-plugin-babel injects the helpers. Can you try using @babel/plugin-transform-runtime and @babel/runtime instead? It should work until the rollup-plugin-babel problem is fixed.

Or you can also try forcing rollup to use @babel/helpers v7.5.0 by npm-installing it.

@benjamn

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

I’m seeing the objectSpread helper injected as a function, even though I’m using @babel/plugin-transform-runtime:

function _objectSpread(target) { for (var i = 1; i < arguments.length; i++) { if (i % 2) { var source = arguments[i] != null ? arguments[i] : {}; var ownKeys = Object.keys(source); if (typeof Object.getOwnPropertySymbols === 'function') { ownKeys = ownKeys.concat(Object.getOwnPropertySymbols(source).filter(function (sym) { return Object.getOwnPropertyDescriptor(source, sym).enumerable; })); } ownKeys.forEach(function (key) { _defineProperty(target, key, source[key]); }); } else { Object.defineProperties(target, Object.getOwnPropertyDescriptors(arguments[i])); } } return target; }

Is that something y’all’re aware of?

benjamn added a commit to meteor/meteor that referenced this pull request Jul 6, 2019
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

Did you tell @babel/plugin-transform-runtime which version of @babel/runtime you are using (using the version option)? Otherwise we must assume that it is ^7.0.0, and can't rely on helpers introduced in newer versions.

@billyjanitsch

This comment has been minimized.

Copy link

commented Jul 6, 2019

@nicolo-ribaudo FYI the version option is not documented in the runtime plugin docs. Would you like me to open an issue for that in the website repo?

Also, if the version option is incompatible with objectSpread2, shouldn't the effect of this PR be to fall back to importing the old objectSpread helper, rather than injecting the new objectSpread2 helper? In other words, it doesn't seem like the try/catch block added in this PR is working as intended.

benjamn added a commit to meteor/babel that referenced this pull request Jul 6, 2019
@benjamn

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

@nicolo-ribaudo That was the trick I needed, thanks!

@billyjanitsch I was just about to leave a similar comment about documentation. You have my full support for opening an issue.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2019

FYI the version option is not documented in the runtime plugin docs. Would you like me to open an issue for that in the website repo?

Thanks! I have opened babel/website#2055 in the meantime, but a PR would be appreciated 😊

Also, if the version option is incompatible with objectSpread2, shouldn't the effect of this PR be to fall back to importing the old objectSpread helper, rather than injecting the new objectSpread2 helper? In other words, it doesn't seem like the try/catch block added in this PR is working as intended.

The try-catch block is used when loading the v7.5 plugin with an older @babel/core version, which doesn't even know about the existence of objectSpread2.
If you are using @babel/core 7.5 the helper exists. @babel/transform-runtime inlines it because it's better to use an inline working helper, than a broken helper from @babel/runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.