rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

RFC: createElement changes and surrounding deprecations

Open sebmarkbage opened this issue 6 years ago • 116 comments

This proposal simplifies how React.createElement works and ultimately lets us remove the need for forwardRef.

  • Deprecate "module pattern" components.
  • Deprecate defaultProps on function components.
  • Deprecate spreading key from objects.
  • Deprecate string refs (and remove production mode _owner field).
  • Move ref extraction to class render time and forwardRef render time.
  • Move defaultProps resolution to class render time.
  • Change JSX transpilers to use a new element creation method.
    • Always pass children as props.
    • Pass key separately from other props.
    • In DEV,
      • Pass a flag determining if it was static or not.
      • Pass __source and __self separately from other props.

The goal is to make element creation as simple as:

function jsx(type, props, key) {
 return {
   $$typeof: ReactElementSymbol,
   type,
   key,
   props,
 };
}

View Rendered Text

sebmarkbage avatar Feb 22 '19 04:02 sebmarkbage

This awesome and actually not scary at all in my opinion :+1: Having to delete both key and ref from props was always a bit unfortunate. I'm wondering if the jsx constructor should accept ref as an argument too.

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.

marvinhagemeister avatar Feb 22 '19 05:02 marvinhagemeister

The original plan was to pass ref separately too but now I’m thinking that ref should just be a normal prop in all cases except when it is passed to a class component. In that case we can strip it off in a shallow clone of the props that excludes it.

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.

sebmarkbage avatar Feb 22 '19 07:02 sebmarkbage

@sebmarkbage Ohh right, now I see where you're getting at. I didn't fully grok the part about forwardRef. Just using the ref prop directly instead of having to create an intermediate forwardRef component is a lot more elegant. This makes this proposal even more exciting :100:

Regarding keys: This proposal is mainly talking about createElement, but I'm wondering if the key extraction would affect cloneElement in the same way. They both share the same type-signature.

marvinhagemeister avatar Feb 22 '19 08:02 marvinhagemeister

@uniqueiniquity @ryancavanaugh @weswigham

DanielRosenwasser avatar Feb 22 '19 08:02 DanielRosenwasser

This is cool, thank you!

So for every set of props, we have to do an expensive dynamic property check to see if there is a key prop in there.

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 hasOwnProperty which can't be expensive

To minimize churn and open up a larger discussion about this syntax

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

NE-SmallTown avatar Feb 22 '19 09:02 NE-SmallTown

Change JSX transpilers to use a new element creation method. Always pass children as props.

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 createElement(type: any, props: object, children: any[]).

A best of both worlds will be for this to only apply to <Component> nodes, i.e <Component>1<Component> would emit createElement(Component, {children: 1}, null) and <h1>1<h1> would emit createElement('h1', {}, [1]) this will avoid arbitrary arity and avoid potentially megamorphic access on the props object by the runtime.

thysultan avatar Feb 22 '19 09:02 thysultan

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 avatar Feb 22 '19 12:02 trueadm

@trueadm That's a great point 👍 One reason we held off on doing this for preact is the various different forms of jsx transpilation. We have quite a few users who just use plain TS and don't use babel.

If the flags idea would proceed, we'd need to agree and standardise on a common set of values. There is obviously the risk of tying it too much to one particular jsx implementation. Looking at the flags in inferno there are several ones that are very specific to them. Same for vue.

Nonetheless it is a very interesting idea and I'd love to see it pursued further 👍

marvinhagemeister avatar Feb 22 '19 15:02 marvinhagemeister

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.

developit avatar Feb 22 '19 15:02 developit

@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 babel-plugin-react, which they probably shouldn't be.

I guess Preact should use babel-plugin-preact or something along those lines (maybe it already does?), in which case this RFC should have no side-effects on other projects. Maybe I'm missing something here too? I know there might be issues with cross-compat support using this API, but I was under the assumption that Inferno and Preact had moved away from trying to be 100% compatible because of things like concurrent rendering and suspense making it almost impossible.

[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.

trueadm avatar Feb 22 '19 15:02 trueadm

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.

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?

trueadm avatar Feb 22 '19 15:02 trueadm

I don't see how this proposal would affect other libraries - unless they use babel-plugin-react

@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 <h1><p>Hello<p>{[name, null, 1]}<h1>. Should produce something akin to:

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 avatar Feb 22 '19 16:02 thysultan

@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?

trueadm avatar Feb 22 '19 16:02 trueadm

@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 React.createElement (well, straight up rename it to something else!)

ferdaber avatar Feb 22 '19 16:02 ferdaber

although it also sounds like you’ll be creating far more virtual objects than we do now?

@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:

  1. Normalisation is isolated to explicit dynamic parts delegated/isolated to a single responsibility function i.e jsx.from/jsxFROM.
  2. Arity is always maintained.
  3. Single responsibility functions, text, empty, node, children that further isolate hot paths in case future internal library changes want to explore other internal data-structure layouts without breaking compact. Which is why the children, text functions in my example are identity functions that return their arguments as is – they could alternatively execute their own logic depending on what data-structure for these types fits the lib.
  4. Monomorphic is maintained as much as possible outside of the dynamic parts.

If the transpiler is more aggressive it could also promote static-like arrays [name, null, 1] in <h1><p>Hello<p>{[name, null, 1]}<h1> to being static.

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))))

thysultan avatar Feb 22 '19 16:02 thysultan

The problem is that not all tooling supports adding new dependencies from a transform. The first step is figuring out how this can be done idiomatically in the current ecosystem.

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 @key, though other ideas that come to mind are either more confusing or would break all current jsx parsers. Probably the least bad one is <Component:constantKey /> / <Component:{expressionKey} /> but that sure looks like it could be confused with xmlns.


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 😝

Jessidhia avatar Feb 22 '19 17:02 Jessidhia

@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 avatar Feb 22 '19 17:02 trueadm

@trueadm There are a few reasons.

  1. Engines can better inline small single responsibility functions, they are effectively free except for the dynamic variant jsx.from which would need branching etc, at which point we are either invoking the same number of functions or less.
  2. This in turn means it is less bytes, because there are less bit flags that are always being passed around, i.e the difference between(manually minified) n('h1', null, c([])) and j(1, 'h1', {children: []}, ...) or t('Hello') and j(3, null, "Hello", ...).
  3. Less branching – with bit flags you would have to pass these(flags) to every callsite this would in turn mean that you would need to execute bitwise operations for every invocation and introduce branching into every call context, with my suggested case each function is a single responsibility citizen so there's less to no branching in the static case, which makes for a good direct threaded dispatch pipeline which is positive feedback for point 1. about making it easier for inlining heuristics.

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 [name, null, 1] array example.

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.

thysultan avatar Feb 22 '19 17:02 thysultan

@thysultan

  1. Engines can better inline small single responsibility functions, they are effectively free except for the dynamic variant jsx.from which would need branching etc, at which point we are either invoking the same number of functions or less.

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.

2. This in turn means it is less bytes, because there are less bit flags that are always being passed around, i.e the difference between(manually minified) n('h1', null, c([])) and j(1, 'h1', {children: []}, ...).

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.

3. Less branching – with bit flags you would have to pass these(flags) to every callsite this would in turn mean that you would need to execute bitwise operations for every invocation and introduce branching into every call context, with my suggested case each function is a single responsibility citizen so there's less to no branching in the static case, which makes for a good direct threaded dispatch pipeline which is positive feedback for point 1. about making it easier for inlining heuristics.

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.

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 archive these. So an attempt to avoid these should be at the helm of any optimisation centric approach.

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.

trueadm avatar Feb 22 '19 18:02 trueadm

@trueadm

Bit wise flags are extremely cheap to do and far more effective than object lookups.

When i mentioned jsx.node or jsx.text this is more a readable presentation of for example jsxNode or jsxText that do not involve object lookups, i'm sorry, that was probably not clear in my previous exchange.

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.

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 jsx.from/jsFrom serve to represent in the examples i posted.

That is assume everything is static unless it's signal'ed as dynamic.

thysultan avatar Feb 22 '19 19:02 thysultan

@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 jsx(...) in production, the compiler could inline an array, with a dynamic signature that still remains fully monomorphic [flags, ...anyInputsDeterminedByFlags].

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.

trueadm avatar Feb 22 '19 19:02 trueadm

@trueadm Yes, the proposed monolith jsx() or granular text/node/empty/children/from could both serve as intermediate representations with a 2nd tier compiler that translates these further down the pipeline. I wonder if the 2nd tier compilers artefacts could be standardised as well given the divergence of implementation details between libraries.

On thing the current proposal will find hard to avoid with/without flags is the mega-morphic access that is props.children from the heuristic of implicitly assigning children to props for host elements(non-component types) instead of a strict Array type parameter...

Because at some point the runtime will need to retrieve props.children to extract children and the props shape is probably going to be all over the place with any real app. This ties into my previous suggestion.

thysultan avatar Feb 22 '19 20:02 thysultan

@thysultan You'd possibly use a flag to indicate it is a host node:

const flags = HOST_COMPONENT | HAS_CHILDREN | HAS_STATIC_PROPS | IS_STATIC;
const flags2 = TEXT_NODE | IS_STATIC;
const flags3 = TEXT_NODE;
[flags, "div", { className: "name-box" }, [[flags2, "Hello world. My name is "], [flags3, props.name]]]

For example... but I'm not saying we have to go that far right now. We should start to think about that now though, so we don't make those compiler optimizations impossible in the future without significant churn.

trueadm avatar Feb 22 '19 20:02 trueadm

@Jessidhia we changed that for Preact X which is right around the corner. children won't always be an array anymore in Preact. We'll go into more detail on our true announcement once it's out 👍

@trueadm love the direction this is taking. I'm on board 👍

marvinhagemeister avatar Feb 23 '19 00:02 marvinhagemeister

@Jessidhia

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'd note that this wouldn't matter at all if children were completely opaque. Which they eventually might become. Traversing children breaks abstraction because usually it makes it impossible to add a component layer in the middle without breaking subtle assumptions. Of course, there are cases like Menu and MenuItem components that want to be aware of children. We think the solution to this isn’t easier introspection (which arrays don’t even help that much with — because those can be nested), but it’s a first-class multipass render API. That’s what Call/Return experiment tried to solve. We scrapped that API because it was confusing but the end game is the same.

gaearon avatar Mar 02 '19 17:03 gaearon

I realized that the key can't really be pass through in the current semantics. It might have to be:

function jsx(type, props, key) {
 return {
   $$typeof: ReactElementSymbol,
   type,
   key: key === undefined ? null : '' + key,
   props,
 };
}

The semantics are quite weird because undefined turns into null but null actually turns into "null" because a defined value is stringified.

This exists mostly to ensure somewhat consistent type signatures of these fields.

We could special case defined vs not defined (like hasOwnProperty or in vs undefined) but the big question is whether a dynamic values such as <div key={somethingUnknown} /> is allowed to have undefined mean missing key.

It might make for a difficult upgrade path to change that given that we can't really know for sure on every key callsite whether it might be undefined. E.g. <div key={keys[i]} />. Stringifying these cases would cause a duplicate key very easily which would be bad.

sebmarkbage avatar Apr 08 '19 18:04 sebmarkbage

Some feedback regarding defaultProps deprecation after migrating around 90 components away from defaultProps:

  1. bundle size went up (+0.05% parsed, +0.50% gzipped). IE11 and chrome 45 don't support object destructuring assigment which doesn't transpile well. I would hope that this will be mitigated by react itself shipping less code to resolve defaultProps.
  2. If a parent wants to read from the props of its children it will not read the default value. This is only possible with defaultProps.
  3. current doc tooling (react-docgen; are there better alternatives?) requires either destructuring in function signature or custom handler
  4. function and class components move farther apart: Before we had a single API for default values. Now we have to consider the component type.
  5. Better typing experience. Latest type versions support defaultProps properly but there are still some edge cases when dealing with higher-order components.

eps1lon avatar Apr 25 '19 12:04 eps1lon

@eps1lon This is great research and feedback!

Some follow up questions:

  1. Do you happen to have a bundle comparison that doesn't transpile the destructuring assignment but still minifies? It would be nice to have a "modern JS" comparison since hopefully people will eventually be able to ship the untranspiled form.
  2. Yea, this is the major limitation which will also apply to class components with this full proposal. The idea is to save as much work as possible at the actual callsites creating JSX. The common case is that the parent doesn't read. Longer term, we'd like to find alternatives so that it's not needed to read the props. That way they could be opaque and bound directly to the function that is going to be called. That way it can eagerly resolve slots instead of materializing a temporary object.
  3. Can you elaborate on the types of patterns that you'd expect to be supported but aren't?

sebmarkbage avatar Apr 25 '19 18:04 sebmarkbage

Can you elaborate on the types of patterns that you'd expect to be supported but aren't?

// A
// default value of `foo` recognized
function Component({ foo = 1, bar }) {
   //
}
// B
// function body is hidden
function Component(props) {
  const { foo = 1, bar } = props;
}

With more props or wrappers like memo or forwardRef pattern A becomes quite ugly. Purely stylistic though so this is at least for me less of an issue. It seems not too hard though for static analysis to find the props destructuring in the function body.

It would be nice to have a "modern JS" comparison since hopefully people will eventually be able to ship the untranspiled form.

I got confused with the chrome version. 41 is the version still relevant though this is more likely to improve in the foreseeable future. ~Leaves~ IE11 and we can only pray that usage drops more. Edit: I'm a bit confused by the behavior of babel-preset-env: I need to drop support for edge < 18, firefox < 53 and chrome < 51. This doesn't match the mdn page nor caniuse. Even the data source lists edge 17 as a browser support object destructuring. Looks like something is outdated. Edit: transform-destructuring is required for destructuring iterators in earlier browsers (like edge 15). We technically don't need it for edge 14 and object destructuring. There's still an issue in babel-preset-env and edge 15 which will be resolved with babel/babel#9902.

Do you happen to have a bundle comparison that doesn't transpile the destructuring assignment but still minifies?

So with

// browserslistrc
// edge >= 15 is enough in a future version of babel
edge >= 18 
firefox >= 53
chrome >= 51
safari >= 10
node 8.0

We get (just want to emphasize that this is not just component code; there's a bit (20%?) noise from 3rd party libraries):

parsed gzipped
before 294_238 B 80_369 B
after 290_300 B 79_655 B
absolute diff -3_938 B -714 B
relative diff -1.3% -0.8%

Which is somewhat expected since default props can't be mangled.

That way it can eagerly resolve slots instead of materializing a temporary object.

Love it. Didn't understand a word :smile:

eps1lon avatar Apr 25 '19 21:04 eps1lon

If ref is going to be part of props, will it be considered by React.memo? If so, what about refs passed as inline functions?

diegohaz avatar Apr 28 '19 17:04 diegohaz