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

Combine ReactJSXElementValidator with main module #28317

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 13, 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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 13, 2024
@acdlite acdlite closed this Feb 13, 2024
@acdlite acdlite reopened this Feb 13, 2024
@acdlite acdlite marked this pull request as ready for review February 13, 2024 16:38
@react-sizebot
Copy link

react-sizebot commented Feb 13, 2024

Comparing: 015ff2e...881e029

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 176.83 kB 176.83 kB = 55.11 kB 55.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 178.83 kB 178.83 kB = 55.69 kB 55.69 kB
facebook-www/ReactDOM-prod.classic.js = 594.81 kB 594.81 kB = 104.91 kB 104.91 kB
facebook-www/ReactDOM-prod.modern.js = 578.10 kB 578.10 kB = 101.93 kB 101.93 kB
oss-experimental/react/cjs/react-jsx-runtime.react-server.production.min.js +2.49% 0.88 kB 0.91 kB +1.64% 0.55 kB 0.56 kB
oss-stable-semver/react/cjs/react-jsx-runtime.react-server.production.min.js +2.49% 0.88 kB 0.91 kB +1.64% 0.55 kB 0.56 kB
oss-stable/react/cjs/react-jsx-runtime.react-server.production.min.js +2.49% 0.88 kB 0.91 kB +1.64% 0.55 kB 0.56 kB
test_utils/ReactAllWarnings.js Deleted 66.26 kB 0.00 kB Deleted 16.32 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react/cjs/react-jsx-runtime.react-server.production.min.js +2.49% 0.88 kB 0.91 kB +1.64% 0.55 kB 0.56 kB
oss-stable-semver/react/cjs/react-jsx-runtime.react-server.production.min.js +2.49% 0.88 kB 0.91 kB +1.64% 0.55 kB 0.56 kB
oss-stable/react/cjs/react-jsx-runtime.react-server.production.min.js +2.49% 0.88 kB 0.91 kB +1.64% 0.55 kB 0.56 kB
oss-experimental/react/cjs/react-jsx-runtime.profiling.js +1.98% 4.08 kB 4.16 kB +2.08% 1.83 kB 1.87 kB
oss-stable-semver/react/cjs/react-jsx-runtime.profiling.js +1.98% 4.08 kB 4.16 kB +2.08% 1.83 kB 1.87 kB
oss-stable/react/cjs/react-jsx-runtime.profiling.js +1.98% 4.08 kB 4.16 kB +2.08% 1.83 kB 1.87 kB
oss-experimental/react/cjs/react-jsx-runtime.production.js +1.98% 4.08 kB 4.16 kB +2.02% 1.83 kB 1.87 kB
oss-stable-semver/react/cjs/react-jsx-runtime.production.js +1.98% 4.08 kB 4.16 kB +2.02% 1.83 kB 1.87 kB
oss-stable/react/cjs/react-jsx-runtime.production.js +1.98% 4.08 kB 4.16 kB +2.02% 1.83 kB 1.87 kB
oss-experimental/react/cjs/react-jsx-runtime.react-server.production.js +1.44% 4.17 kB 4.23 kB +0.96% 1.87 kB 1.89 kB
oss-stable-semver/react/cjs/react-jsx-runtime.react-server.production.js +1.44% 4.17 kB 4.23 kB +0.96% 1.87 kB 1.89 kB
oss-stable/react/cjs/react-jsx-runtime.react-server.production.js +1.44% 4.17 kB 4.23 kB +0.96% 1.87 kB 1.89 kB
facebook-react-native/react/cjs/JSXRuntime-prod.js +0.42% 1.43 kB 1.43 kB = 0.70 kB 0.70 kB
facebook-react-native/react/cjs/JSXRuntime-profiling.js +0.42% 1.43 kB 1.43 kB = 0.70 kB 0.70 kB
facebook-react-native/react/cjs/JSXRuntime-dev.js +0.21% 41.09 kB 41.18 kB +0.05% 10.79 kB 10.79 kB
oss-stable-semver/react/cjs/react-jsx-dev-runtime.development.js = 46.22 kB 45.66 kB = 13.47 kB 13.28 kB
oss-stable/react/cjs/react-jsx-dev-runtime.development.js = 46.22 kB 45.66 kB = 13.47 kB 13.28 kB
oss-experimental/react/cjs/react-jsx-dev-runtime.development.js = 46.19 kB 45.63 kB = 13.46 kB 13.27 kB
facebook-www/JSXDEVRuntime-dev.classic.js = 54.17 kB 53.51 kB = 14.00 kB 13.80 kB
facebook-www/JSXDEVRuntime-dev.modern.js = 54.17 kB 53.51 kB = 14.00 kB 13.80 kB
facebook-react-native/react/cjs/JSXDEVRuntime-dev.js = 40.43 kB 39.77 kB = 10.60 kB 10.41 kB
test_utils/ReactAllWarnings.js Deleted 66.26 kB 0.00 kB Deleted 16.32 kB 0.00 kB

Generated by 🚫 dangerJS against 881e029

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.

export {REACT_FRAGMENT_TYPE as Fragment, jsx, jsxs};
export {REACT_FRAGMENT_TYPE as Fragment, jsx, jsxs, jsxDEV};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we weren't going to support jsxDEV in react-server. Maybe I misremembered or misunderstood?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just copy pasta, I'll fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

"it's defined in, or you might have mixed up default and named imports.";
}

const sourceInfo = getSourceInfoErrorAddendum(source);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The use of the source should really go away completely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll submit a separate PR to delete that

/**
* 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@acdlite acdlite Feb 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Did you find any discrepancies in the implementations that this changes?

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 20, 2024

Did you find any discrepancies in the implementations that this changes?

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.

@acdlite acdlite merged commit ec160f3 into facebook:main Feb 20, 2024
36 checks passed
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)
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
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants