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

Fixed a regression for multiple __self & __source props with classic runtime #12475

Merged
merged 3 commits into from Dec 10, 2020

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Dec 10, 2020

Q                       A
Fixed Issues? fixes #12253 (comment)
Patch: Bug Fix? x
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This most likely fixes the regression reported here: #12253 (comment) . The old dev transforms did not have a problem with inserting those props multiple times - there is no detection for this in example here:

node.attributes.push(t.jsxAttribute(id, t.jsxExpressionContainer(trace)));


for (const attr of attribs) {
const name =
t.isJSXAttribute(attr) &&
t.isJSXIdentifier(attr.name) &&
attr.name.name;

if (name === "__source" || name === "__self") {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is part of the code that will be removed in Babel 8 anyway, so I didn't have to provide any extra TODO comments or anything here

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow: why would this be removed? I think we'll keep the classic runtime, and just make the automatic one the default.

@Andarist explained me on Slack that this is for the !useSpread case, that we are removing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - this is just in the compat path that handles useSpread and useBuiltIns options. It's OK to still throw this error when one uses automatic runtime.

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 10, 2020

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 10, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fce9a05:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo added area: react i: regression PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Dec 10, 2020
@Andarist Andarist force-pushed the fix/jsx-dev-preset-regression branch from f0c4432 to 93ca1be Compare December 10, 2020 11:21
@Andarist Andarist force-pushed the fix/jsx-dev-preset-regression branch from 93ca1be to da01efe Compare December 10, 2020 11:25
@@ -1,4 +1,5 @@
{
"skip": true,
Copy link
Member

Choose a reason for hiding this comment

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

If this doesn't work (I have never seen this used before), you'll need to rename the folder to .classic-runtime-with-source-self-plugins-windows (with .) instead.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, instead of disabling this test we could remove ...props from the fixture input, and make it only throw in Babel 8?

Copy link
Member Author

Choose a reason for hiding this comment

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

It didn't :trollface: was worth trying!

@Andarist Andarist force-pushed the fix/jsx-dev-preset-regression branch from 2ff309a to 3b0d4c1 Compare December 10, 2020 11:48
@Andarist
Copy link
Member Author

The CI is failing but it doesn't seem to be related 🤔

@nicolo-ribaudo
Copy link
Member

Yeah, also other PRs are failing with a 500 error today.

@nicolo-ribaudo nicolo-ribaudo merged commit 1ef9e19 into main Dec 10, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the fix/jsx-dev-preset-regression branch December 10, 2020 17:17
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: react i: regression 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants