-
Notifications
You must be signed in to change notification settings - Fork 558
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
RFC: createElement changes and surrounding deprecations #107
base: main
Are you sure you want to change the base?
Conversation
28558db
to
c71cb88
Compare
This awesome and actually not scary at all in my opinion 👍 Having to delete both function jsx(type, props, key, ref) {
return {
$$typeof: ReactElementSymbol,
type,
key,
ref,
props,
};
} Pinging @DanielRosenwasser as this will have implications on the built-in jsx transpilation in TypeScript. |
The original plan was to pass The original reason for treating ref separately was because it easy to accidentally spread it along (and before that transferPropsTo). The normal pattern for spread is that you should combine it with rest to pick off any properties you consume. let [myCustomProp, ...domStuff] = props;
doSomething(myCustomProp);
return <div {...domStuff} />; The problem is that you never “do” anything with ref on a class component. It has already been automatically consumed in React. So if ref was part of props, then in this pattern it is easy to forget to avoid passing along the ref. Which is extra bad because refs really should only be attached to one thing at a time. However if you want to attach a ref on a function component you do need to explicitly do something with the ref. let [myCustomProp, ref, ...domStuff] = props;
doSomething(myCustomProp);
useImperativeHandle(ref, ...);
return <div {...domStuff} />; So the reason for special casing ref no longer exists in the Hooks world. In fact, the reason it is special cases makes it worse in the Hooks world because you need to use forwardRef instead of just a plain function component. Therefore I think the right call here is to stop special casing it at the element level and instead start special casing it only for classes. |
|
||
We'd add a warning in createElement if something that doesn't have `.prototype.isReactComponent` on it uses `defaultProps`. This includes other special components like `forwardRef` and `memo`. | ||
|
||
It's trickier to upgrade if you're passing the whole props object around but you can always reconstruct it if needed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my reading, the React Fire Ticket #13525 seemed to suggest that destructuring is not really a preferred pattern here (classname
being renamed to class
, giving destructuring of class
a beginner-unfriendly syntax).
While I'm all in favor of default props being deprecated in favor of default props & destructuring there seems to be a slight conflict here in terms of simplicity & education of new developers.
I have no real opinion of either way, just wanted to point it out early in the thought process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. Tbh I’ve grown to favor keeping className
instead of class
for similar reasons.
The plan for Fire is more lava than set in stone.
@sebmarkbage Ohh right, now I see where you're getting at. I didn't fully grok the part about Regarding keys: This proposal is mainly talking about |
This is cool, thank you!
I'm a bit confused why this is expensive? Seems we just do the check in DEV and even if in DEV it just call
I don't think this is a good idea/start for React, we don't introduce any syntax like this('@'), all syntaxes we import is normal javascript syntax, but '@' is not(except decorator but that situation is not appropriate for this) I would suggest that just make this become a breaking change because this spread usage is not popular(even is just edge case) from my perspective |
This will probably break other libraries that use JSX(unless it's configurable) and would still require a poly/megamorphic access for the runtime to access children props for host elements. It might be safer to define a strict arity(3) for A best of both worlds will be for this to only apply to |
} | ||
``` | ||
|
||
However, in function components there really isn't much need for this pattern since you can just use JS default arguments and all the places where you typically use these values are within the same scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes for a more consistent API though. It doesn't matter what at what particular elementType I'm looking at currently. All I know is that a static property called defaultProps
is responsible for default values.
Makes (human and machine) parsing of a given component definition much easier: "Just" scan for defaultProps
.
This is also tricky for doc generators like react-docgen
. It currently only supports defaultProps if destructuring is happening in the function signature. This might lead to an opinionated pattern how function component signatures should look like e.g.
function Component(props) {
const { optionalProp = 'defaulted' } = props;
}
does not work currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eps1lon As far as I know the plan is to deprecate class-based components entirely in the long run. Viewed from that perspective the decision to remove defaultProps
or the other things in this RFC make a lot more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecate=split them into own module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can't meaningfully "deprecate" class components (and don't intend to) in the observable future. However we do want to de-emphasize them if the Hooks adoption is successful and growing. Then simplifying the modern API at the cost of some clumsiness in the de-emphasized API seems justified.
```js | ||
import {jsx} from "react"; | ||
function Foo() { | ||
return jsx('div', ...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of importing import {jsx} from "react"
, have you considered injecting the jsx
?
function Foo() {
return jsx => jsx('div', ...);
}
or
function Foo(props, jsx) {
return jsx('div', ...);
}
Also, have you considered calling it h
, instead of jsx
, like Vue, Preact and the rest of the community does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsx makes more sense to me. The rest of the community isn’t strictly true - Inferno uses createVNode, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trueadm Inferno is 100x less popular than Vue and Preact. And there are at least 10 more libs that use h
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@streamich Where did you get the 100x figure from? Why are you even comparing popularity for? It's completely irrelevant for what we're looking for. Inferno influenced many of today's frameworks and libraries in the ecosystem - including React, Preact and Vue 3. That accounts for much more than Github stars or npm downloads in my eyes. My reference to Inferno was specific to this RFC too. createVNode
in Inferno was designed purely for JSX compilation, not for users to hand-write. For those wanting to write UIs without JSX, the option to use something like hyperscript is still available (it just wraps around internal APIs).
There is a reason why h
is used, and that's because it was originally short for hyperscript
which has a specific API that was adopted by virtual DOM frameworks. https://github.com/Raynos/virtual-hyperscript. This differs from what this RFC is trying to do, as the signature will no longer be hyperscript or even hyperscript-like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trueadm Your point about the different signature is good, other points not so much.
But is is the signature that much different? The signature of various h
across the libraries are quite a bit different, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@streamich The core team aren't looking to leverage hyperscript though. We're looking for a better signature for creating React elements, specifically from JSX input. I don't see jsx
as something people will be manually using, which was why hyperscript came around in the first place (to be a better alternative to createElement for building UIs in non-JSX libraries).
There's nothing stopping people from creating a wrapping library that offers an API like h
that wraps around this new API. Like I said in my original point, there's a reason why not every library uses h
, and my given example was Inferno. It was nothing to do with popularity, it was because of consumption. You never write createVNode
in Inferno, it's got an API very much like the one proposed in this RFC, in where it's purely created from JSX compilation. You'd use h
or createElement
in Inferno, if you were writing the nodes manually by hand (without JSX). Those APIs are just wrappers around createVNode
. I'm suggesting the exact same hypothesis here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this definitely warrants it's own function name. We will want to export an h() shim that calls jsx() internally if this moves forward. h() would imply hyperscript compat and I don't think it would be a good idea to break that assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core team aren't looking to leverage hyperscript though. We're looking for a better signature for creating React elements, specifically from JSX input.
OK, I see your point and it makes perfect sense. And I see you proposed signature with flags, which also makes sense:
It's just from reading this RFC, the proposed signature
is pretty much what some libraries call h
, with maybe exception for third key
argument.
But if you add flags
there, then you are right, then it definitely does not make sense to call it h
,
text/0000-create-element-changes.md
Outdated
- We don't know if the passed in props is a user created object that can be mutated so we must always clone it once. | ||
- `key` and `ref` gets extracted from JSX props provided so even if we didn't clone, we'd have to delete a prop, which would cause that object to become map-like. | ||
- `key` and `ref` can be spread in dynamically so without prohibitive analysis, we don't know if these patterns will include them `<div {...props} />`. | ||
- The transform relies on a the name `React` being in scope of JSX. I.e. you have to import the default. This is unfortunate as more things like Hooks are typically used as named arguments. Ideally, you wouldn't have to important anything to use JSX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The transform relies on a the name `React` being in scope of JSX. I.e. you have to import the default. This is unfortunate as more things like Hooks are typically used as named arguments. Ideally, you wouldn't have to important anything to use JSX. | |
- The transform relies on a the name `React` being in scope of JSX. I.e. you have to import the default. This is unfortunate as more things like Hooks are typically used as named imports. Ideally, you wouldn't have to important anything to use JSX. |
I feel like this would be the perfect opportunity to introduce flags to the ‘jsx’ function call signature too. It would mean altering this RFC slightly, but just like how Inferno, Ivi and Vue 3 do, they use the concept of bitwise flags to contain information about the virtual node. For example, the flags could easily tell the reconciler if there are keyed children, or if the props are static, avoiding unnecessary diffs and further improving update performance. This would obviously require the JSX compiler to insert this information, but it’s proven highly beneficial to other libraries. I’d imagine the signature might be: jsx(flags, props, type, key) There’s plenty of prior research and art into how this works and how it voids unnecessary lookups, diffs, and computation. Instead you’d just do a bitwise operation to find if a given condition holds. |
@trueadm That's a great point 👍 One reason we held off on doing this for If the Nonetheless it is a very interesting idea and I'd love to see it pursued further 👍 |
Huge +1 to adding flags, but with extreme caution: it should be a bitmask of values that indicated JSX source assumptions, not library optimizations - that's the only way this could work across frameworks. It could start simple - static props, static subtree, static children, is element, is component. All of these have unfortunate bailouts when spread or direct jsx() calls are used, so we can only ever hope to use them as informative. One tiny nit - flags should be optional, so it seems like they should be an optional 4th argument instead of the 1st. |
@marvinhagemeister I'm not sue this would need to be standardized - there are no changes to the JSX spec. Furthermore, I don't see how this proposal would affect other libraries - unless they use I guess Preact should use [Update] I understand what you mean now, this makes more sense to me. The Inferno flags are loosely framework independent, but I agree a proper set of independent flags would be beneficial to other libraries and frameworks too. |
I'd be happy with this too. Would make a lot of sense and I guess this is what @marvinhagemeister meant originally. Although, I'm still unsure of why these need to be compatible in the plugin level? Aren't libraries using their own Babel/TS transforms these days? |
My proposal is that we solve this by treating a static `key` prop as different from one provided through spread. I think that as a second step we might want to even give separate syntax such as: | ||
|
||
```js | ||
<div @key="Hi" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about react:key
? It clarifies that it’s a React-specific prop that’s not passed to the component.
@trueadm They do use the pragma option provided: https://babeljs.io/docs/en/babel-plugin-transform-react-jsx and this is not uncommon/taboo since it is often easier to use the pragma configuration of the tools default plugin than to integrate a custom third-party plugin. {
"plugins": [
["@babel/plugin-transform-react-jsx", {
"pragma": "dom", // default pragma is React.createElement
"pragmaFrag": "DomFrag", // default is React.Fragment
"throwIfNamespace": false // defaults to true
}]
]
} My only other suggestion with regards to the JSX to JS generation is that this should aim to make iterating and normalising over static children obsolete. That is jsx.node('h1', null, jsx.children([
jsx.node('p', null, jsx.text('Hello')),
jsx.from([name, null, 1])
]))
function node (type, props, children) { return {type, props, children} }
function children (arr) {return arr}
function text (val) { return val }
function from (val) { /* check typeof dynamic value */ } |
@thysultan I was under the impression that pragma would still work as it does now, except it would use the current createElement signature? You would just reference babel-plugin-legacy-react or something. Your idea is quite neat, although it also sounds like you’ll be creating far more virtual objects than we do now? |
@Jessidhia this might be a great time for us to actually perform a major breaking change on React types, since this overturns quite a lot of the assumptions around JSX type checking, and we'd need to change the signatures of |
@trueadm The example doesn't create any more objects than what is currently done/what this proposal plans towards. It however does tries to ensure that:
If the transpiler is more aggressive it could also promote static-like arrays jsx.node('h1', null, jsx.children([
- jsx.node('p', null, jsx.text('Hello')),
+ jsx.node('p', null, jsx.text('Hello', -4294967295 /* index-hash-as-implicit-key */)),
- jsx.from([name, null, 1])
+ jsx.children([jsx.from(name), jsx.empty(/* key */), jsx.text(1, /* key */)])
]))
+ function text(val, key) { /* ... */ }
+ function empty(key) { /* ... */ } ..and bail out as needed for more complex cases, it should at the very least be flexible enough to improve on such cases as needed, i.e It could probably play it safe and bail out of: <h1>{data.map(v => v)}</h1>
jsx.node('h1', null, jsx.children([jsx.from(data.map(v => v))])) or play it strict/loose and... <h1>{data.map(v => v)}</h1>
jsx.node('h1', null, jsx.children(data.map(v => jsx.from(v)))) |
Babel 7 definitely can, logan made helpers for it that are also capable of telling between ES or commonjs modules and injects import or require appropriately, and can be reused from any plugin. This probably won't fly with people that still use babel-standalone in the browser for example but I wonder if that's used for anything beyond toy projects. Not a big fan of It'd also be nice do deal with the duality of children being sometimes a value, sometimes an array, but that'd require a React 18. Preact has children always as arrays. I'll probably make more comments after I've slept because it is 2am 😝 |
@thysultan so you’d use flags under the hood? If this isn’t going to be something you have write, then why have all these helper methods at all and skip the intermediate abstraction and just do a single call with flags passed in? It will result in less bytes in the app code and you will not need many function calls. I’m not against your API, I just don’t see any advantages to it cause just inlining bitwise flags that contain more information, using less bytes. |
@trueadm There are a few reasons.
At the root of it, my suggestion tips this on its head so that instead of assuming everything as dynamic and passing flags to signal static tree's assume everything is static and isolate dynamic trees, you can then pass as many flags as needed to only these dynamic parts or improve the transpiler incrementally to better understand the static parts within outwardly looking dynamic parts as demonstrated in the At the end of the day walking the whole tree is a walk in the park(pun intended) compared to the cost of generating these virtual snapshots, and i don't mean creating the end products(virtual objects), but the tangled soup of dealing with arbitrary arity, normalisation and the necessary branching need to achieve these. So an attempt to avoid these should be at the helm of any optimisation centric approach. |
This isn't always the case and it's generally much better to use a single function. Essentially, it really depends on the JS engine and how much of exhaustive the inline cache is. You can easily show JITs performing well on these cases in microbenchmarks, but in real-world applications with several hundred kb of JS, you'll find the inline cache gets consumed very quickly.
That's true, but in reality you're trading a function call vs passing an inline object literal, which would be offset by gzip/brotli compression.
Bit wise flags are extremely cheap to do and far more effective than object lookups, property lookups, switch statements. Plus they generally get optimized at a CPU level, because they're just arithmetic operations. I'm happy to dig more into this if you're interested in knowing more.
I think we're aiming for the same thing here. The issue is that we have to be careful as to how we approach this now. We want to get this right and we want to enable future long-term compiler optimizations without over-complicating what we need in the short-term. This idea really isn't about walking the tree faster, it's far simpler than that - it's how we encode additional information into React Element objects when compiled from JSX. I'm suggesting we add a "flags" field to represent this data, rather than having several objects or several fields that essentially do the same thing. The next point, is why do we need this additional information? Well, at build time, we generally have lots of information that is never accessible at runtime. For example, if we have access to TypeScript or Flow annotations at build time, and that information is passed to a JSX element, we could use that information to infer that a component never re-renders or that the props of a component never change. By encoding this information into a field, we can substantially reduce the actual computation and memory usage at runtime. |
When i mentioned
That's that i was trying to convey with the ethos of my suggestion, bitwise flags are by their nature less future-flexible you have to agree ahead of time. That said i'm not against flags, just that flags-if-any should be delegated exclusively to the dynamic parts and should not inherently pollute the pipeline of static representations, which is what That is assume everything is static unless it's signal'ed as dynamic. |
@thysultan Thanks for making that part clear. I understand you better now. I think one of the core parts of why I feel so strong for flags, is that it can remove all function calls entirely in production mode. Rather than using I've been using this approach for some experimentation with React compilation (full compilation, using type annotations) and it's extremely efficient both with a JIT and also highly efficient without one (important for low-end devices that don't use a JIT due to memory constraints). This is testing this approach on real-world apps with hundreds of thousands of objects are created, not just in synthetic benchmarks. |
|
||
In a future major, we'll remove string refs and that will let us get rid of the `_owner` field from elements. | ||
|
||
We have a transform that adds `__self`. We can use this to issue different warnings when `__self` and `_owner` have the same value. In those cases it is safe to run an automated codemod of string refs from `ref="foo"` to `ref={n => this.refs.foo = n}`. Therefore the recommendation becomes to first fix all the cases where `__self` and `_owner` are *different* since those need manual intervention. That warning can go out earlier. After that warning has taken effect, we can next tell people to run a codemod for the rest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does requires a change in React since this.refs
is a sealed object in DEV unless we use string refs. Which means the codemod can't be applied in versions where string refs are deprecated (we already have a deprecation warning in StrictMode
trees). Unless we change the codemod to use
ref={(current) => {
if (process.env.NODE_ENV !== 'production') {
if (Object.isSealed(this.refs)) {
this.refs = {}
}
}
this.refs.stringRef = current;
}}
instead: https://codesandbox.io/s/correct-string-refs-codemod-yejy2e?file=/src/App.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also just unseal it in versions where you're supposed to apply the codemod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just about unsealing I realized. React classes share the same object in this.refs
until we attach string refs. So the proper codemod would apply
const emptyRefsObject = new React.Component().refs;
...
ref={(current) => {
if (this.refs === emptyRefsObject) {
// This is a lazy pooled frozen object, so we need to initialize.
this.refs = {};
}
if (current === null) {
delete this.refs.stringRef;
} else {
this.refs.stringRef = current;
}
}}
to ensure the same behavior as string refs.
Or we would ensure that each new react class component instance has its own refs object by default
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@sebmarkbage , @eps1lon, @gaearon, is there any plan to implement this in the near future? At Microsoft, we use React heavily and have noticed that We've tested a change where the compiler picks a new Since the scope of this RFC is very large, is it possible to break this RFC into two phases where in phase 1 we fix the performance issues that don't cause breaking changes and integrate them into the jsx compilers, and in the following phase 2 we include the other deprecations that @sebmarkbage has proposed to make I'm proposing that for phase 1 we create a new It will look something like this: -export function createElement(type, config, children) {
+export function createFastElement(type, config, children) {
let propName;
-
- // Reserved names are extracted
- const props = {};
-
+ let props = config;
let key = null;
let ref = null;
let self = null;
let source = null;
- if (config != null) {
- if (hasValidRef(config)) {
- ref = config.ref;
+ if (props != null) {
+ if (hasValidRef(props)) {
+ ref = props.ref;
if (__DEV__) {
- warnIfStringRefCannotBeAutoConverted(config);
+ warnIfStringRefCannotBeAutoConverted(props);
}
}
- if (hasValidKey(config)) {
- key = '' + config.key;
+ if (hasValidKey(props)) {
+ key = '' + props.key;
}
- self = config.__self === undefined ? null : config.__self;
- source = config.__source === undefined ? null : config.__source;
- // Remaining properties are added to a new props object
- for (propName in config) {
- if (
- hasOwnProperty.call(config, propName) &&
- !RESERVED_PROPS.hasOwnProperty(propName)
- ) {
- props[propName] = config[propName];
+ self = props.__self === undefined ? null : props.__self;
+ source = props.__source === undefined ? null : props.__source;
+
+ // Reserved properties are deleted in place
+ for (propName in RESERVED_PROPS) {
+ if (hasOwnProperty.call(props, propName)) {
+ delete props.propName;
}
}
+ } else {
+ props = {}
}
... For Case 1 the compiler will continue to use the existing For Case 2 the compiler will use the |
The problem we have is that there's not one single uniformly used compiler for React. It takes a long time for a compiler change to trickle through the community. It was only recently that people started adopting the new one. The point of the new one is that the same compiler is compatible with the end state. If we released this intermediate step, people would have to switch away from the compiler that supports the end state to a new one that doesn't. We'd at least have to structure it so that it's compatible with the end state. However, even if we did that, we'd end up with an extra intermediate runtime that we'd have to support indefinitely even after shipping the end state because it would take a long time to phase out the intermediate state compiler. That is in addition to any further runtime support for the larger React Forget compiler project which will also add to go further. |
@sebmarkbage for an intermediary performance fix, what if we modified the existing Could we use the Option 1: @@ -2,10 +2,11 @@ export function createElement(type, config, children) {
let propName;
// Reserved names are extracted
- const props = {};
+ let { key, ref, __source, __self, ...props } = config || {};
+
+ key = null;
+ ref = null;
- let key = null;
- let ref = null;
let self = null;
let source = null;
@@ -23,15 +24,6 @@ export function createElement(type, config, children) {
self = config.__self === undefined ? null : config.__self;
source = config.__source === undefined ? null : config.__source;
- // Remaining properties are added to a new props object
- for (propName in config) {
- if (
- hasOwnProperty.call(config, propName) &&
- !RESERVED_PROPS.hasOwnProperty(propName)
- ) {
- props[propName] = config[propName];
- }
- }
} Could we also simply assign the config object to the props object when the props object does not need to get modified? Option 2: @@ -2,17 +2,22 @@ export function createElement(type, config, children) {
let propName;
// Reserved names are extracted
- const props = {};
+ let props = {};
let key = null;
let ref = null;
let self = null;
let source = null;
+ let allowPropsToReferenceConfig = true;
+
+ // Children can be more than one argument, and those are transferred onto
+ // the newly allocated props object.
+ const childrenLength = arguments.length - 2;
+
if (config != null) {
if (hasValidRef(config)) {
ref = config.ref;
-
if (__DEV__) {
warnIfStringRefCannotBeAutoConverted(config);
}
@@ -23,20 +28,31 @@ export function createElement(type, config, children) {
self = config.__self === undefined ? null : config.__self;
source = config.__source === undefined ? null : config.__source;
- // Remaining properties are added to a new props object
- for (propName in config) {
- if (
- hasOwnProperty.call(config, propName) &&
- !RESERVED_PROPS.hasOwnProperty(propName)
- ) {
- props[propName] = config[propName];
+
+ for (propName in RESERVED_PROPS) {
+ if (hasOwnProperty.call(config, propName)) {
+ allowPropsToReferenceConfig = false;
}
}
+
+ if (type && type.defaultProps) {
+ allowPropsToReferenceConfig = false;
+ }
+
+ if (childrenLength > 0) {
+ allowPropsToReferenceConfig = false;
+ }
+
+ if (allowPropsToReferenceConfig) {
+ props = config;
+ } else {
+ // Remaining properties are added to a new props object
+ // eslint-disable-next-line no-unused-vars
+ const { key: _key, ref: _ref, __self, __source, ...rest } = config;
+ props = rest;
+ }
}
- // Children can be more than one argument, and those are transferred onto
- // the newly allocated props object.
- const childrenLength = arguments.length - 2;
if (childrenLength === 1) {
props.children = children;
} else if (childrenLength > 1) { I took a benchmark of each case and saw that they were much faster than the current https://codepen.io/zbogard/pen/QWzpwjy In Chrome 116: |
One reason we don't tend to rely on modern features in React itself is because people end up using compilers downstream that lowers it into a worse and unpredictable version than React itself does. However, at the downside of not being able to use a native version if it ends up faster. However, I will say that just shipping the real breaking change here isn't that hard for someone with context. We just don't have anyone with context working on it right now. So instead of having them focus on an intermediate, they could just ship the real thing that's even better. |
- We need to do a dynamic test against a component if it has a `.defaultProps` during every element creation call. This can't be optimized well because the function it is called within is highly megamorphic. | ||
- `.defaultProps` in element creation doesn't work with `React.lazy` so in that case we also have to check for resolving defaultProps in the render phase too, and means that the semantics are inconsistent anyway. | ||
- Children are passed as var args and we have to patch them onto props dynamically instead of statically knowning the shape of the props at the callsite. | ||
- The transform uses `React.createElement` which is a dynamic property look up instead of a constant closed over module scope. This minimizes poorly and takes a little cost to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this currently only applies when not also transformed to CommonJS. Both SWC and Babel will use _jsxruntime.jsx()
when the CommonJS transform is applied.
Only Babel playground offers a shareabled link to highlight the issue but you can verify the same behavior with SWC (https://swc.rs/playground) when editing the JSON config directly.
It works as intended without any module transforms though.
This proposal simplifies how React.createElement works and ultimately lets us remove the need for forwardRef.
key
from objects._owner
field).ref
extraction to class render time andforwardRef
render time.defaultProps
resolution to class render time.key
separately from other props.__source
and__self
separately from other props.The goal is to make element creation as simple as:
View Rendered Text