Combine ReactJSXElementValidator with main module #28317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
force-pushed
the
inline-jsx-element-validator
branch
from
f6f54b8
to
14fd963
Compare
acdlite
force-pushed
the
inline-jsx-element-validator
branch
from
f6f54b8
to
6af435e
Compare
acdlite
force-pushed
the
inline-jsx-element-validator
branch
from
6af435e
to
4def47f
Compare
There are too many layers to the JSX runtime implementation. I think basically everything should be implemented in a single file, so that's what I'm going to do. As a first step, this deletes ReactJSXElementValidator and moves all the code into ReactJSXElement. I can already see how this will help us remove more indirections in the future. Next I'm going to do start moving the `createElement` runtime into this module as well, since there's a lot of duplicated code.
acdlite
force-pushed
the
inline-jsx-element-validator
branch
from
4def47f
to
881e029
Compare
This was referenced Feb 13, 2024
gnoff
reviewed
Feb 14, 2024
|
|
||
| export {REACT_FRAGMENT_TYPE as Fragment, jsx, jsxs}; | ||
| export {REACT_FRAGMENT_TYPE as Fragment, jsx, jsxs, jsxDEV}; |
There was a problem hiding this comment.
I thought we weren't going to support jsxDEV in react-server. Maybe I misremembered or misunderstood?
There was a problem hiding this comment.
just copy pasta, I'll fix
There was a problem hiding this comment.
Seems like it doesn't really hurt as long as it's only in DEV. We're not hard deprecating the transform. It's just unnecessary and later we can hard deprecate.
sebmarkbage
reviewed
Feb 19, 2024
| "it's defined in, or you might have mixed up default and named imports."; | ||
| } | ||
|
|
||
| const sourceInfo = getSourceInfoErrorAddendum(source); |
There was a problem hiding this comment.
The use of the source should really go away completely.
There was a problem hiding this comment.
I'll submit a separate PR to delete that
sebmarkbage
reviewed
Feb 19, 2024
| /** | ||
| * https://github.com/reactjs/rfcs/pull/107 | ||
| * @param {*} type | ||
| * @param {object} props | ||
| * @param {string} key | ||
| */ | ||
| export function jsxDEV(type, config, maybeKey, source, self) { | ||
| export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { |
There was a problem hiding this comment.
Other than the soon-to-be-gone-self warning, this shouldn't really be used anymore so the relationship should probably be the inverse where jsxDEV helper calls jsx or jsxs internally for backwards compat but doesn't have its own implementation.
There was a problem hiding this comment.
Yeah that was how I started but since the implementation jsx and jsxs are identical in dev right now except for one branch, I was calling a shared function anyway and it ended up being about the same thing. But I'll keep this in mind. I think I can refactor this function so that check isn't so deep and we can get rid of the boolean argument. But I didn't want to do any extra refactoring in this initial PR to minimize the risk of an accidental behavior change.
sebmarkbage
approved these changes
Feb 19, 2024
No but I did find several instances of code that wasn't being used anywhere, as a result of the JSX runtime originally being a copy-paste of createElement. I'm guessing the code must have been shuffled around a few times in the original PR that landed it and nobody noticed. That was partly what motivated me to open this PR in the first place, because I kept getting confused by all the layering. |
github-actions bot
pushed a commit
that referenced
this pull request
Feb 20, 2024
There are too many layers to the JSX runtime implementation. I think basically everything should be implemented in a single file, so that's what I'm going to do. As a first step, this deletes ReactJSXElementValidator and moves all the code into ReactJSXElement. I can already see how this will help us remove more indirections in the future. Next I'm going to do start moving the `createElement` runtime into this module as well, since there's a lot of duplicated code. DiffTrain build for [ec160f3](ec160f3)
acdlite
added a commit
that referenced
this pull request
Feb 20, 2024
Depends on: - #28317 --- There's a ton of overlap between the createElement implementation and the JSX implementation, so I combined them into a single module. In the actual build output, the shared code between JSX and createElement will get duplicated anyway, because react/jsx-runtime and react (where createElement lives) are separate, flat build artifacts. So this is more about code organization — with a few key exceptions, the implementations of createElement and jsx are highly coupled.
github-actions bot
pushed a commit
that referenced
this pull request
Feb 20, 2024
Depends on: - #28317 --- There's a ton of overlap between the createElement implementation and the JSX implementation, so I combined them into a single module. In the actual build output, the shared code between JSX and createElement will get duplicated anyway, because react/jsx-runtime and react (where createElement lives) are separate, flat build artifacts. So this is more about code organization — with a few key exceptions, the implementations of createElement and jsx are highly coupled. DiffTrain build for [5fb2c93](5fb2c93)
acdlite
added a commit
that referenced
this pull request
Feb 20, 2024
Depends on: - #28317 - #28320 --- Changes the behavior of the JSX runtime to pass through `ref` as a normal prop, rather than plucking it from the props object and storing on the element. This is a breaking change since it changes the type of the receiving component. However, most code is unaffected since it's unlikely that a component would have attempted to access a `ref` prop, since it was not possible to get a reference to one. `forwardRef` _will_ still pluck `ref` from the props object, though, since it's extremely common for users to spread the props object onto the inner component and pass `ref` as a differently named prop. This is for maximum compatibility with existing code — the real impact of this change is that `forwardRef` is no longer required. Currently, refs are resolved during child reconciliation and stored on the fiber. As a result of this change, we can move ref resolution to happen only much later, and only for components that actually use them. Then we can remove the `ref` field from the Fiber type. I have not yet done that in this step, though.
github-actions bot
pushed a commit
that referenced
this pull request
Feb 20, 2024
Depends on: - #28317 - #28320 --- Changes the behavior of the JSX runtime to pass through `ref` as a normal prop, rather than plucking it from the props object and storing on the element. This is a breaking change since it changes the type of the receiving component. However, most code is unaffected since it's unlikely that a component would have attempted to access a `ref` prop, since it was not possible to get a reference to one. `forwardRef` _will_ still pluck `ref` from the props object, though, since it's extremely common for users to spread the props object onto the inner component and pass `ref` as a differently named prop. This is for maximum compatibility with existing code — the real impact of this change is that `forwardRef` is no longer required. Currently, refs are resolved during child reconciliation and stored on the fiber. As a result of this change, we can move ref resolution to happen only much later, and only for components that actually use them. Then we can remove the `ref` field from the Fiber type. I have not yet done that in this step, though. DiffTrain build for [fa2f82a](fa2f82a)
This was referenced Feb 20, 2024
huozhi
added a commit
to vercel/next.js
that referenced
this pull request
Feb 23, 2024
### React upstream changes - facebook/react#28333 - facebook/react#28334 - facebook/react#28378 - facebook/react#28377 - facebook/react#28376 - facebook/react#28338 - facebook/react#28331 - facebook/react#28336 - facebook/react#28320 - facebook/react#28317 - facebook/react#28375 - facebook/react#28367 - facebook/react#28380 - facebook/react#28368 - facebook/react#28343 - facebook/react#28355 - facebook/react#28374 - facebook/react#28362 - facebook/react#28344 - facebook/react#28339 - facebook/react#28353 - facebook/react#28346 - facebook/react#25790 - facebook/react#28352 - facebook/react#28326 - facebook/react#27688 - facebook/react#28329 - facebook/react#28332 - facebook/react#28340 - facebook/react#28327 - facebook/react#28325 - facebook/react#28324 - facebook/react#28309 - facebook/react#28310 - facebook/react#28307 - facebook/react#28306 - facebook/react#28315 - facebook/react#28318 - facebook/react#28226 - facebook/react#28308 - facebook/react#27563 - facebook/react#28297 - facebook/react#28286 - facebook/react#28284 - facebook/react#28275 - facebook/react#28145 - facebook/react#28301 - facebook/react#28224 - facebook/react#28152 - facebook/react#28296 - facebook/react#28294 - facebook/react#28279 - facebook/react#28273 - facebook/react#28269 - facebook/react#28376 - facebook/react#28338 - facebook/react#28331 - facebook/react#28336 - facebook/react#28320 - facebook/react#28317 - facebook/react#28375 - facebook/react#28367 - facebook/react#28380 - facebook/react#28368 - facebook/react#28343 - facebook/react#28355 - facebook/react#28374 - facebook/react#28362 - facebook/react#28344 - facebook/react#28339 - facebook/react#28353 - facebook/react#28346 - facebook/react#25790 - facebook/react#28352 - facebook/react#28326 - facebook/react#27688 - facebook/react#28329 - facebook/react#28332 - facebook/react#28340 - facebook/react#28327 - facebook/react#28325 - facebook/react#28324 - facebook/react#28309 - facebook/react#28310 - facebook/react#28307 - facebook/react#28306 - facebook/react#28315 - facebook/react#28318 - facebook/react#28226 - facebook/react#28308 - facebook/react#27563 - facebook/react#28297 - facebook/react#28286 - facebook/react#28284 - facebook/react#28275 - facebook/react#28145 - facebook/react#28301 - facebook/react#28224 - facebook/react#28152 - facebook/react#28296 - facebook/react#28294 - facebook/react#28279 - facebook/react#28273 - facebook/react#28269 Closes NEXT-2542 Disable ppr test for strict mode for now, @acdlite will check it and we'll sync again
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
There are too many layers to the JSX runtime implementation. I think basically everything should be implemented in a single file, so that's what I'm going to do. As a first step, this deletes ReactJSXElementValidator and moves all the code into ReactJSXElement. I can already see how this will help us remove more indirections in the future. Next I'm going to do start moving the `createElement` runtime into this module as well, since there's a lot of duplicated code.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Depends on: - facebook#28317 --- There's a ton of overlap between the createElement implementation and the JSX implementation, so I combined them into a single module. In the actual build output, the shared code between JSX and createElement will get duplicated anyway, because react/jsx-runtime and react (where createElement lives) are separate, flat build artifacts. So this is more about code organization — with a few key exceptions, the implementations of createElement and jsx are highly coupled.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Depends on: - facebook#28317 - facebook#28320 --- Changes the behavior of the JSX runtime to pass through `ref` as a normal prop, rather than plucking it from the props object and storing on the element. This is a breaking change since it changes the type of the receiving component. However, most code is unaffected since it's unlikely that a component would have attempted to access a `ref` prop, since it was not possible to get a reference to one. `forwardRef` _will_ still pluck `ref` from the props object, though, since it's extremely common for users to spread the props object onto the inner component and pass `ref` as a differently named prop. This is for maximum compatibility with existing code — the real impact of this change is that `forwardRef` is no longer required. Currently, refs are resolved during child reconciliation and stored on the fiber. As a result of this change, we can move ref resolution to happen only much later, and only for components that actually use them. Then we can remove the `ref` field from the Fiber type. I have not yet done that in this step, though.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
There are too many layers to the JSX runtime implementation. I think basically everything should be implemented in a single file, so that's what I'm going to do. As a first step, this deletes ReactJSXElementValidator and moves all the code into ReactJSXElement. I can already see how this will help us remove more indirections in the future. Next I'm going to do start moving the `createElement` runtime into this module as well, since there's a lot of duplicated code. DiffTrain build for commit ec160f3.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
Depends on: - #28317 --- There's a ton of overlap between the createElement implementation and the JSX implementation, so I combined them into a single module. In the actual build output, the shared code between JSX and createElement will get duplicated anyway, because react/jsx-runtime and react (where createElement lives) are separate, flat build artifacts. So this is more about code organization — with a few key exceptions, the implementations of createElement and jsx are highly coupled. DiffTrain build for commit 5fb2c93.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
Depends on: - #28317 - #28320 --- Changes the behavior of the JSX runtime to pass through `ref` as a normal prop, rather than plucking it from the props object and storing on the element. This is a breaking change since it changes the type of the receiving component. However, most code is unaffected since it's unlikely that a component would have attempted to access a `ref` prop, since it was not possible to get a reference to one. `forwardRef` _will_ still pluck `ref` from the props object, though, since it's extremely common for users to spread the props object onto the inner component and pass `ref` as a differently named prop. This is for maximum compatibility with existing code — the real impact of this change is that `forwardRef` is no longer required. Currently, refs are resolved during child reconciliation and stored on the fiber. As a result of this change, we can move ref resolution to happen only much later, and only for components that actually use them. Then we can remove the `ref` field from the Fiber type. I have not yet done that in this step, though. DiffTrain build for commit fa2f82a.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are too many layers to the JSX runtime implementation. I think basically everything should be implemented in a single file, so that's what I'm going to do.
As a first step, this deletes ReactJSXElementValidator and moves all the code into ReactJSXElement. I can already see how this will help us remove more indirections in the future.
Next I'm going to do start moving the
createElementruntime into this module as well, since there's a lot of duplicated code.