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 parameters or JSX dev runtime #3880

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Fix parameters or JSX dev runtime #3880

merged 1 commit into from
Feb 3, 2023

Conversation

wooorm
Copy link
Contributor

@wooorm wooorm commented Feb 3, 2023

My guess is that the previous PR “fixed” the earlier problem because self isn’t used, so by calling isStaticChildrenself”, a bug went away.

The source for where this jsxDEV call is generated in Babel is here: https://github.com/babel/babel/blob/3952486/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts#L506-L508.
The React RFC for the transform that mentions the dev runtime is here: https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#dev-only-transforms.
React’s implementation is here: https://github.com/facebook/react/blob/855b77c9bbee347735efcd626dda362db2ffae1d/packages/react/src/jsx/ReactJSXElementValidator.js#L306-L311

* isStaticChildren is the same as whether jsxs would be used, instead of jsx. Which is also whether there are 2 or more children passed:

  • <a /> -> jsx('a', {})
  • <a>b</a> -> jsx('a', {children: 'b'})
  • <a>{1}{2}</a> -> jsxs('a', {children: [1, 2]})

Related-to: GH-3459.

* There was a parameter missing (`isStaticChildren: boolean`), which is not useful\*,
  but is still being passed
* Fix order of `source` and `self` again (incorrectly introduced in GH-3459)
* Fix some (internal JSDoc) types for these parameters

My guess is that the previous PR “fixed” the earlier problem because `self` isn’t used, so by
calling `isStaticChildren` “`self`”, a bug went away.

The source for where this `jsxDEV` call is generated in Babel is here:
<https://github.com/babel/babel/blob/3952486/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts#L506-L508>.
The React RFC for the transform that mentions the dev runtime is here:
<https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#dev-only-transforms>

\* `isStaticChildren` is the same as whether `jsxs` would be used, instead of `jsx`.
Which is also whether there are 2 or more children passed:

*   `<a />` -> `jsx('a', {})`
*   `<a>b</a>` -> `jsx('a', {children: 'b'})`
*   `<a>{1}{2}</a>` -> `jsxs('a', {children: [1, 2]})`

Related-to: GH-3459.
@wooorm
Copy link
Contributor Author

wooorm commented Feb 3, 2023

/cc @marvinhagemeister and @JoviDeCroock who worked on / reviewed the previous PR!

@coveralls
Copy link

Coverage Status

Coverage: 99.531%. Remained the same when pulling 56452dd on wooorm:fix-jsx-dev into fc5758b on preactjs:master.

@JoviDeCroock JoviDeCroock merged commit 5eecaf1 into preactjs:master Feb 3, 2023
@JoviDeCroock
Copy link
Member

@wooorm thank you for all the pointers, made it much easier to review this!

@marvinhagemeister
Copy link
Member

@wooorm Good catch, that's on me for messing up the arguments. Thank you so much for filing a PR!

@wooorm wooorm deleted the fix-jsx-dev branch February 3, 2023 11:36
JoviDeCroock added a commit that referenced this pull request Jan 12, 2024
JoviDeCroock added a commit that referenced this pull request Jan 12, 2024
* backport #3871

* port test from #3884 functionality seems to work in v11

* backport #3880

* backport #3875

* backport #3862

* add todo for #3801

* backport #3868

* backport #3867

* backport #3863

* add todo for #3856

* backport #3844

* backport #3816

* backport #3888

* backport #3889
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants