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
Conversation
Can you add a comment that the old helper can be removed in the next major release? |
No idea what's causing failure to rename. |
Inside the helper or above |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10860/ |
It seems somehow the helper name can't include a trailing number? |
Yeah it can. It will be renamed to |
Can this be merged or do this have any more blockers? |
Any news for this? @nicolo-ribaudo |
@nicolo-ribaudo How about we just treat |
It wouldn't work, because the old helper handles all the arguments the same way. |
The previous issue was that the old code didn’t work with the new helper because the helper assumed that the odd arguments are all objects. Modifying that behavior to also accept null/undefined should fix the specific issue.
…________________________________
From: Nicolò Ribaudo <notifications@github.com>
Sent: Friday, February 22, 2019 6:36:38 PM
To: babel/babel
Cc: Kagami Sascha Rosylight; Author
Subject: Re: [babel/babel] Retry to fix object spread helper compatibility (#9384)
It wouldn't work, because the old helper handles all the arguments the same way.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fbabel%2Fbabel%2Fpull%2F9384%23issuecomment-466335255&data=02%7C01%7C%7C9686f159844d40ee11e308d698a93f15%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636864250000910849&sdata=BhdClNGJWM%2B0itD%2BogKAw1M5v4voWPBzabl%2BWvQvY2I%3D&reserved=0>, or mute the thread<https://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FADPUToTEfwqohLJAG1TEVNBo8uFKeHj2ks5vP7omgaJpZM4aMNXC&data=02%7C01%7C%7C9686f159844d40ee11e308d698a93f15%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636864250000920854&sdata=RJCsefcY99I5dj9IdLlm7dTWluR5ABuFJXWC2TgMbqA%3D&reserved=0>.
|
helpers.objectSpread2 = helper("7.0.0-beta.0")` | ||
import defineProperty from "defineProperty"; | ||
|
||
export default function _objectSpread(target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a default export, the name doesn't matter.
Anyway, we should rename it for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does generateUidIdentifier("objectSpread2")
, which removes 2
before generating the id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would objectSpreadTwo
is okay or should I fix the function to not remove 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok as it is now for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean this can be ignored?
Anyway, we should rename it for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename the helper:
export default function _objectSpread2
But is is not a problem if that 2
then gets automatically removed by generateUid
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long wait. Can you please rebase the PR, so that it can be merged?
…babel#9341)" (babel#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>
Thanks! |
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! |
This change breaks IE11 compatibility when using object spread syntax without using polyfills as getOwnPropertyDescriptors is not available in IE11. |
Could you please open a new issue? 🙏 |
I got the error
There must be something wrong in ADD: |
Same error as referenced by @edi9999 |
We're getting a similar error after upgrading from "Unknown helper objectSpread2". |
Yeah, add |
@QingLeiLi Can you please share your entire package.json file ? |
package.json ? Do you mean babel config?
|
Just wanted to mention that the issue I was describing above was fixed by this PR: #10170 |
Retries #9341.
This adds
helpers._objectSpread2()
where only the even arguments would be considered as spreads.