Skip to content

Fix jsxDEV for generated elements#12017

Merged
JLHwung merged 6 commits into
babel:mainfrom
Timer:hotfix/generated-jsx
Aug 31, 2020
Merged

Fix jsxDEV for generated elements#12017
JLHwung merged 6 commits into
babel:mainfrom
Timer:hotfix/generated-jsx

Conversation

@Timer

@Timer Timer commented Aug 28, 2020 •
edited by gitpod-io Bot
Loading

Copy link
Copy Markdown
Contributor
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

codesandbox-ci Bot commented Aug 28, 2020 •
edited
Loading

Copy link
Copy Markdown

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

babel-bot commented Aug 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

@Timer Timer mentioned this pull request Aug 28, 2020
Merged
@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
hzoo
hzoo approved these changes Aug 28, 2020
View reviewed changes

@hzoo hzoo left a comment

Copy link
Copy Markdown
Member

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

lunaruan commented Aug 28, 2020

Copy link
Copy Markdown
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!

JLHwung
JLHwung reviewed Aug 28, 2020
View reviewed changes
Comment thread packages/babel-helper-builder-react-jsx-experimental/src/index.js Outdated
JLHwung
JLHwung requested changes Aug 28, 2020
View reviewed changes

@JLHwung JLHwung left a comment

Copy link
Copy Markdown
Contributor

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

Timer commented Aug 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
@JLHwung JLHwung force-pushed the hotfix/generated-jsx branch from 9fb3f24 to e166df3 Compare August 31, 2020 14:24
@Timer Timer requested a review from JLHwung August 31, 2020 18:32
JLHwung
JLHwung approved these changes Aug 31, 2020
View reviewed changes

@JLHwung JLHwung left a comment

Copy link
Copy Markdown
Contributor

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

Timer commented Aug 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@JLHwung

JLHwung commented Aug 31, 2020

Copy link
Copy Markdown
Contributor

@Timer I will do a release later today.

@JLHwung

JLHwung commented Aug 31, 2020

Copy link
Copy Markdown
Contributor

Fixed in @babel/[email protected].

@Timer

Timer commented Aug 31, 2020

Copy link
Copy Markdown
Contributor Author

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.

Reviewers

@existentialism existentialism existentialism approved these changes
@hzoo hzoo hzoo approved these changes
@JLHwung JLHwung JLHwung approved these changes
+1 more reviewer
@gaearon gaearon gaearon approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Timer @babel-bot @lunaruan @JLHwung @existentialism @hzoo @gaearon