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

Retry to fix object spread helper compatibility #9384

Merged
merged 1 commit into from May 25, 2019

Conversation

@saschanaz
Copy link
Contributor

commented Jan 22, 2019

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

Retries #9341.

This adds helpers._objectSpread2() where only the even arguments would be considered as spreads.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Can you add a comment that the old helper can be removed in the next major release?

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

No idea what's causing failure to rename.

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Can you add a comment that the old helper can be removed in the next major release?

Inside the helper or above helpers.objectSpread line?

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2019

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

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

It seems somehow the helper name can't include a trailing number?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Yeah it can. It will be renamed to _objectSpread when inlined but it isn't a problem: @babel/runtime/helpers/objectSpread will still point to the old helper and @babel/runtime/helpers/objectSpread2 to the new one.

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Can this be merged or do this have any more blockers?

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

Any news for this? @nicolo-ribaudo

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@nicolo-ribaudo How about we just treat null/undefined always as {}? That way we can keep the compatibility with the previous behavior without adding objectSpread2.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

It wouldn't work, because the old helper handles all the arguments the same way.

@saschanaz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

helpers.objectSpread2 = helper("7.0.0-beta.0")`
import defineProperty from "defineProperty";
export default function _objectSpread(target) {

This comment has been minimized.

Copy link
@danez

danez Mar 1, 2019

Member

Should it be named objectSpread2? I wonder in the case someone uses babel-runtime, this might be a problem, but I don't know exactly how runtime works.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 3, 2019

Member

Since this is a default export, the name doesn't matter.
Anyway, we should rename it for consistency.

This comment has been minimized.

Copy link
@danez

danez Mar 3, 2019

Member

But where is the name in generated output coming from? Because right now it is called _objectSpread even with he new helper, which made me think it might collide in the runtime again.

I thought it might be the name of the function here.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 3, 2019

Member

It does generateUidIdentifier("objectSpread2"), which removes 2 before generating the id.

This comment has been minimized.

Copy link
@saschanaz

saschanaz Mar 13, 2019

Author Contributor

Would objectSpreadTwo is okay or should I fix the function to not remove 2?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 13, 2019

Member

It's ok as it is now for me.

This comment has been minimized.

Copy link
@saschanaz

saschanaz Mar 13, 2019

Author Contributor

You mean this can be ignored?

Anyway, we should rename it for consistency.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 13, 2019

Member

We should rename the helper:

export default function _objectSpread2

But is is not a problem if that 2 then gets automatically removed by generateUid.

@danez
danez approved these changes May 25, 2019
Copy link
Member

left a comment

Sorry for the long wait. Can you please rebase the PR, so that it can be merged?

packages/babel-helpers/src/helpers.js Outdated Show resolved Hide resolved
@danez danez added this to the v7.5.0 milestone May 25, 2019
@buildsize

This comment has been minimized.

Copy link

commented May 25, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.28 MB 2.05 MB -235.61 KB (10%)
babel-preset-env.min.js 1.31 MB 1.1 MB -213.93 KB (16%)
babel.js 2.82 MB 2.77 MB -46.25 KB (2%)
babel.min.js 1.56 MB 1.55 MB -18.31 KB (1%)
…#9341)" (#9379)"

This reverts commit 43b83f8.

Fix objectSpread helper breaking old codes

remove tests to regenerate later

renamed output

new name

try using word

add comment as requested

revert inline name changes

add 2 for consistency

Update packages/babel-helpers/src/helpers.js

Co-Authored-By: Daniel Tschinder <daniel@tschinder.de>
@saschanaz saschanaz force-pushed the saschanaz:spread-fix branch from 56aafa8 to 2d3ce65 May 25, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit a596da2 into babel:master May 25, 2019
5 checks passed
5 checks passed
babel/repl REPL preview is available
Details
buildsize Significant change of babel.min.js up by 825 bytes (0.01%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.41% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 25, 2019

Thanks!

@mathieutu

This comment has been minimized.

Copy link

commented Jun 1, 2019

Hi folks. Thanks for you work.

Just to be sure, do you confirm that this PR will fix the issue #8749?

Do you have any idea of its release planning?

Thanks!

@cloudkite

This comment has been minimized.

Copy link

commented Jul 5, 2019

This change breaks IE11 compatibility when using object spread syntax without using polyfills as getOwnPropertyDescriptors is not available in IE11.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Could you please open a new issue? 🙏

@QingLeiLi

This comment has been minimized.

Copy link

commented Jul 5, 2019

I got the error

ModuleNotFoundError: Module not found: Error: Can't resolve '@babel/runtime/helpers/objectSpread2'

There must be something wrong in babel-plugin-proposal-object-rest-spread.

ADD:objectSpread2 was add in the @babel/runtime-corejs2 package, but the helperGenerator method will search the helper in the @babel/runtime package only, except set the corejs option to 2.

@pwnreza

This comment has been minimized.

Copy link

commented Jul 5, 2019

WhatsApp Image 2019-07-05 at 13 24 32

Same error as referenced by @edi9999
We are depending on building our RN app via pre-installed set of node_modules.
So we are pretty desperate for a fix 🙏

@kimroen

This comment has been minimized.

Copy link

commented Jul 5, 2019

I got the error

ModuleNotFoundError: Module not found: Error: Can't resolve '@babel/runtime/helpers/objectSpread2'

There must be something wrong in babel-plugin-proposal-object-rest-spread.

ADD:objectSpread2 was add in the @babel/runtime-corejs2 package, but the helperGenerator method will search the helper in the @babel/runtime package only, except set the corejs option to 2.

We're getting a similar error after upgrading from 7.4.3 to 7.5.0. The error we're getting is this:

"Unknown helper objectSpread2".

@QingLeiLi

This comment has been minimized.

Copy link

commented Jul 5, 2019

I got the error

ModuleNotFoundError: Module not found: Error: Can't resolve '@babel/runtime/helpers/objectSpread2'

There must be something wrong in babel-plugin-proposal-object-rest-spread.
ADD:objectSpread2 was add in the @babel/runtime-corejs2 package, but the helperGenerator method will search the helper in the @babel/runtime package only, except set the corejs option to 2.

We're getting a similar error after upgrading from 7.4.3 to 7.5.0. The error we're getting is this:

"Unknown helper objectSpread2".

Yeah, add @babel/plugin-transform-runtime option
{ corejs: 2, }
make it works again.

@Udbhav12

This comment has been minimized.

Copy link

commented Jul 5, 2019

@QingLeiLi Can you please share your entire package.json file ?

@QingLeiLi

This comment has been minimized.

Copy link

commented Jul 8, 2019

@QingLeiLi Can you please share your entire package.json file ?

package.json ? Do you mean babel config?

// babel.config.js
module.exports = {
        presets: [
            [
                '@babel/preset-env',
                {
                    modules: 'false',
                    targets: {
                        ios: '9',
                        android: '4.4'
                    },
                    useBuiltIns: 'usage'
                }
            ],
            ['@babel/preset-react'],
            ['@babel/preset-typescript']
        ],
        plugins: [
            ['@babel/plugin-transform-runtime', {
            	// point
                "corejs": 2,
            }],
            '@babel/plugin-proposal-class-properties',
            [
                "@babel/plugin-proposal-decorators",
                {
                    "decoratorsBeforeExport": false
                }
            ]
        ],
};
@pwnreza

This comment has been minimized.

Copy link

commented Jul 8, 2019

WhatsApp Image 2019-07-05 at 13 24 32

Same error as referenced by @edi9999
We are depending on building our RN app via pre-installed set of node_modules.
So we are pretty desperate for a fix 🙏

See here

@kimroen

This comment has been minimized.

Copy link

commented Jul 8, 2019

Just wanted to mention that the issue I was describing above was fixed by this PR: #10170

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