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 jsxDEV for generated elements #12017

Merged
merged 6 commits into from Aug 31, 2020
Merged

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Aug 28, 2020

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

The new JSX transform does not support generated JSX in development mode, it errors with the following:

Property expression of JSXExpressionContainer expected node to be of a type ["Expression","JSXEmptyExpression"] but instead got undefined
    at Object.validate (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/types/lib/definitions/utils.js:132:11)
    at validateField (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/types/lib/validators/validate.js:24:9)
    at validate (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/types/lib/validators/validate.js:17:3)
    at builder (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/types/lib/builders/builder.js:38:27)
    at Object.jsxExpressionContainer (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/types/lib/builders/generated/index.js:848:31)
    at PluginPass.JSXOpeningElement (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/helper-builder-react-jsx-experimental/lib/index.js:53:68)
    at NodePath._call (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:55:20)
    at NodePath.call (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:42:17)
    at NodePath.visit (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/core/node_modules/@babel/traverse/lib/path/context.js:90:31)
    at TraversalContext.visitQueue (/Users/joe/Documents/Development/Work/zeit/next.js/node_modules/@babel/core/node_modules/@babel/traverse/lib/context.js:112:16)

This is caused by the makeSource function returning undefined when location information is absent, instead of skipping the property like the old transform.

This PR mimics the old behavior and omits the __source property when location information is not available. We'll need to check with the React team if this is the desired behavior.

cc @lunaruan


x-ref #11154

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 28, 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 94720de:

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

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 28, 2020

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

@Timer Timer marked this pull request as ready for review August 28, 2020 06:23
@existentialism existentialism added area: react PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Aug 28, 2020
Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

glad the exec test works!

@lunaruan
Copy link
Contributor

Is it OK if we cut a patch for this PR? We're planning on releasing React 17 in a couple of weeks which includes the React entrypoints for the new JSX transform changes. Thanks!

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

I am hesitate on throwing an error when loc is not defined. It is likely that some plugins are not compatible here. But throwing an error in this case blocks compiling JSX. I don't think we should throw here: If we could be tolerant about such issues, the worst outcome is developers don't get correct JSX source locations. It does not worth throwing since the error is not vital.

@Timer
Copy link
Contributor Author

Timer commented Aug 30, 2020

I am hesitate on throwing an error when loc is not defined. It is likely that some plugins are not compatible here.

The plugin currently throws an error when loc is not defined, which as you said, is not compatible with other plugins which is why Next.js cannot upgrade to the new transform.

This PR fixes the transform's compatibility with other plugins!

But throwing an error in this case blocks compiling JSX. I don't think we should throw here: If we could be tolerant about such issues, the worst outcome is developers don't get correct JSX source locations. It does not worth throwing since the error is not vital.

Correct! This PR changes the plugin to not throw an error when source information is missing. You might be referring to the invariant that's present for if the plugin itself improperly uses the makeSource function, which is not exposed as part of the plugin's API.

@Timer Timer requested a review from JLHwung August 30, 2020 16:18
@Timer Timer requested a review from JLHwung August 31, 2020 18:32
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Thanks.

@JLHwung JLHwung merged commit 371e152 into babel:main Aug 31, 2020
@Timer Timer deleted the hotfix/generated-jsx branch August 31, 2020 19:18
@Timer
Copy link
Contributor Author

Timer commented Aug 31, 2020

Do you have an ETA of when this will be released? If not today, we can fork this package temporarily. Thank you!

@JLHwung
Copy link
Contributor

JLHwung commented Aug 31, 2020

@Timer I will do a release later today.

@JLHwung
Copy link
Contributor

JLHwung commented Aug 31, 2020

Fixed in @babel/helper-builder-react-jsx-experimental@7.11.5.

@Timer
Copy link
Contributor Author

Timer commented Aug 31, 2020

Great, thanks!

@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 Dec 1, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: react 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

7 participants