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

RFC: createElement changes and surrounding deprecations #107

Open
wants to merge 3 commits into
base: master
from

Conversation

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Feb 22, 2019

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 sebmarkbage force-pushed the createlement-rfc branch from 28558db to c71cb88 Feb 22, 2019
@marvinhagemeister

This comment has been minimized.

Copy link

marvinhagemeister commented Feb 22, 2019

This awesome and actually not scary at all in my opinion 👍 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.

@sebmarkbage

This comment has been minimized.

Copy link
Collaborator Author

sebmarkbage commented Feb 22, 2019

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.


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:

This comment has been minimized.

Copy link
@phryneas

phryneas Feb 22, 2019

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.

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Feb 22, 2019

Author Collaborator

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.

@marvinhagemeister

This comment has been minimized.

Copy link

marvinhagemeister commented Feb 22, 2019

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

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.

@DanielRosenwasser

This comment has been minimized.

@NE-SmallTown

This comment has been minimized.

Copy link

NE-SmallTown commented Feb 22, 2019

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

@thysultan

This comment has been minimized.

Copy link

thysultan commented Feb 22, 2019

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 thysultan mentioned this pull request Feb 22, 2019
}
```

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.

This comment has been minimized.

Copy link
@eps1lon

eps1lon Feb 22, 2019

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.

This comment has been minimized.

Copy link
@marvinhagemeister

marvinhagemeister Feb 22, 2019

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

This comment has been minimized.

Copy link
@TrySound

TrySound Feb 22, 2019

Contributor

Deprecate=split them into own module

This comment has been minimized.

Copy link
@gaearon

gaearon Feb 22, 2019

Member

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', ...);

This comment has been minimized.

Copy link
@streamich

streamich Feb 22, 2019

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?

This comment has been minimized.

Copy link
@trueadm

trueadm Feb 22, 2019

Member

jsx makes more sense to me. The rest of the community isn’t strictly true - Inferno uses createVNode, for example.

This comment has been minimized.

Copy link
@streamich

streamich Feb 22, 2019

@trueadm Inferno is 100x less popular than Vue and Preact. And there are at least 10 more libs that use h.

This comment has been minimized.

Copy link
@trueadm

trueadm Feb 22, 2019

Member

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

This comment has been minimized.

Copy link
@streamich

streamich Feb 22, 2019

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

This comment has been minimized.

Copy link
@trueadm

trueadm Feb 22, 2019

Member

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

This comment has been minimized.

Copy link
@developit

developit Feb 22, 2019

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.

This comment has been minimized.

Copy link
@streamich

streamich Feb 22, 2019

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:

image

It's just from reading this RFC, the proposed signature

image

image

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 Show resolved Hide resolved
- 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.

This comment has been minimized.

Copy link
@vkrol

vkrol Feb 22, 2019

Suggested change
- 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.
text/0000-create-element-changes.md Outdated Show resolved Hide resolved
@trueadm

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

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.

@marvinhagemeister

This comment has been minimized.

Copy link

marvinhagemeister commented Feb 22, 2019

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

@developit

This comment has been minimized.

Copy link

developit commented Feb 22, 2019

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.

@trueadm

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

@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

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

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?

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" />

This comment has been minimized.

Copy link
@j-f1

j-f1 Feb 22, 2019

How about react:key? It clarifies that it’s a React-specific prop that’s not passed to the component.

@thysultan

This comment has been minimized.

Copy link

thysultan commented Feb 22, 2019

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 */ }
@trueadm

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

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

@ferdaber

This comment has been minimized.

Copy link

ferdaber commented Feb 22, 2019

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

@thysultan

This comment has been minimized.

Copy link

thysultan commented Feb 22, 2019

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

This comment has been minimized.

Copy link

Jessidhia commented Feb 22, 2019

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 😝

@trueadm

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

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

@thysultan

This comment has been minimized.

Copy link

thysultan commented Feb 22, 2019

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

@trueadm

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

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

@thysultan

This comment has been minimized.

Copy link

thysultan commented Feb 22, 2019

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

@trueadm

This comment has been minimized.

Copy link
Member

trueadm commented Feb 22, 2019

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

@ljharb

This comment has been minimized.

Copy link

ljharb commented Aug 13, 2019

For what it's worth, I consider this.props.children to be a mistake, and I'd rather see the children prop deprecated in favor of the third argument to createElement/the child position in jsx. Children are fundamentally and conceptually different than props, and further increasing the conflation between the two will imo harm the usefulness of jsx.

@sompylasar

This comment has been minimized.

Copy link

sompylasar commented Aug 13, 2019

For what it's worth, I consider this.props.children to be a mistake, and I'd rather see the children prop deprecated in favor of the third argument to createElement/the child position in jsx. Children are fundamentally and conceptually different than props, and further increasing the conflation between the two will imo harm the usefulness of jsx.

Any prop, including but not limited to the "children" prop, can contain a React Element that in turn can be put into some other elements' props, including but not limited to "children" prop.

Props are inputs to a component that produces elements as output, "children" is one of such inputs.

@ljharb Where do you see the fundamental difference? How would you propose to consume "children" from within a component if it's a special input, not one of props?

@ljharb

This comment has been minimized.

Copy link

ljharb commented Aug 13, 2019

@sompylasar jsx is where i see the fundamental difference. Without children being special, there's no point in the custom syntax in the first place.

As for how I propose to consume it, we have this.props, this.state, and this.context, why not this.children?

@sompylasar

This comment has been minimized.

Copy link

sompylasar commented Aug 13, 2019

As for how I propose to consume it, we have this.props, this.state, and this.context, why not this.children?

Maybe because we move away from "this.": function Foo(props): React.Node is a React Component.

@ljharb

This comment has been minimized.

Copy link

ljharb commented Aug 13, 2019

@sompylasar sure. but <X {...props}><Y /></X> also becomes React.createElement(X, props, <Y />), so it's all up for potential change.

@sebmarkbage

This comment has been minimized.

Copy link
Collaborator Author

sebmarkbage commented Aug 13, 2019

Other than the namespace to access them what do you expect to be different?

The most compelling argument for separating children to me (which jordwalke has also mentioned) is more of an empirical heuristic. React relies a lot on referential identity and bailing out using paths. That approach is most efficient with an S-expression form. E.g. everything is a tuple. However, in practice things tend to change together and there's overhead in allocations so there is value in flattening the data structures into something like props.

In practice, it happens that it's quite common for values inside children to change while other props doesn't. In this case it would be useful to reuse the old props instance. A tuple of props and children means you can reuse the props while changing the children, which then means you can bailout on the props as a whole unit instead of individually.

It's not always that they change independently. Quite often they change together or maybe there are no props other than children. It can also happen that other props contain JSX elements that are effectively children and those cases could benefit from the same thing. So there's nothing strictly special about children. This argument hedges on the idea that this special case would occur often enough to justify everyone paying for an extra field/argument.

@karelhala karelhala mentioned this pull request Aug 29, 2019
@Elanhant Elanhant mentioned this pull request Sep 4, 2019
4 of 4 tasks complete
@jacobp100

This comment has been minimized.

Copy link

jacobp100 commented Sep 4, 2019

@sebmarkbage what are your current thoughts on ref extraction? I know if your second comment you talked about it just being a regular prop, like children. But in the experimental code, it seems to have special logic

@ljharb

This comment has been minimized.

Copy link

ljharb commented Sep 4, 2019

@sebmarkbage my mental model here doesn’t consider performance at all, but what the things are. The sole value of jsx notation is the mechanism for declaring children, and children are not the same thing as props/attributes - they’re conceptually special. So it feels incumbent on react to make the mental model performant, rather than trying to force mental models for performance.

@BPScott BPScott mentioned this pull request Sep 19, 2019
0 of 6 tasks complete
@sebmarkbage sebmarkbage mentioned this pull request Oct 4, 2019
1 of 12 tasks complete
@MonPote MonPote mentioned this pull request Oct 4, 2019
@rayepps rayepps mentioned this pull request Oct 9, 2019
9 of 9 tasks complete
mrchief added a commit to dowjones/react-dropdown-tree-select that referenced this pull request Oct 15, 2019

# Motivation

In React 0.12 time frame we did a bunch of small changes to how `key`, `ref` and `defaultProps` works. Particularly, they get resolved early on in the `React.createElement(...)` call. This made sense when everything was classes, but since then, we've introduced function components. Hooks have also make function components more prevalent. It might be time to reevaluate some of those designs to simplify things (at least for function components).

This comment has been minimized.

Copy link
@elektronik2k5

elektronik2k5 Oct 20, 2019

Suggested change
In React 0.12 time frame we did a bunch of small changes to how `key`, `ref` and `defaultProps` works. Particularly, they get resolved early on in the `React.createElement(...)` call. This made sense when everything was classes, but since then, we've introduced function components. Hooks have also make function components more prevalent. It might be time to reevaluate some of those designs to simplify things (at least for function components).
In React 0.12 time frame we did a bunch of small changes to how `key`, `ref` and `defaultProps` works. Particularly, they get resolved early on in the `React.createElement(...)` call. This made sense when everything was classes, but since then, we've introduced function components. Hooks have also made function components more prevalent. It might be time to reevaluate some of those designs to simplify things (at least for function components).
@edwardgalligan

This comment has been minimized.

Copy link

edwardgalligan commented Dec 2, 2019

there's nothing strictly special about children. This argument hedges on the idea that this special case would occur often enough to justify everyone paying for an extra field/argument.

I'd like to chime in and echo @ljharb's sentiments here that the raison d'être of JSX is to have this as a conceptual "special-case", or rather to differentiate between the intent of props -vs- children.

While your points are all sound from the point of view of the React internals, and how it has been structured from a performance standpoint, that's not something directly in line with users of JSX as a public API. Without the props/child distinction, the argument for using JSX over directly nesting function calls degrades. XML is fundamentally a representation of triples, not tuples, and JSX is fundamentally an XML-inspired syntax; this is what differentiates it from the Javascript syntax.

Any prop, including but not limited to the "children" prop, can contain a React Element that in turn can be put into some other elements' props, including but not limited to "children" prop.

This is technically true, but pointless and—arguably—conceptually broken. Compare the following:

const X = ({ kids }) => (<div>{kids}</div>);
const Y = ({ children }) => (<div>{children}</div>);

The X example is possible and usable, but what's the point. Applying it is limiting:

...
const header = <h1>Hi</h1>;
return <X kids={[header]} />
...
// vs
...
return <Y><h1>Hi</h1></Y>;

I don't know exactly why JSX was created, but the latter example is in my mind the reason it's used. Just because you can pass components as non-children-keyed props and render them as children of subcomponents, doesn't make that necessarily a good idea for users of React or JSX. Yes it can have it's edge use cases, but they tend to be at a level of component abstraction where the concepts behind JSX-usage are too many layers removed from rendering for it to matter.

This change to children would also cause the elegantly loose coupling between React and JSX to become much tighter. It would not only break any alternative use of JSX, it would remove a lot of the current alternative use-cases, effectively changing it from a syntax designed for React to a syntax designed for React-only.

On the other hand, I would give a 👍 to the suggestion to remove the current special-case of children-as-props-key, in favour of children being isolated as something dedicated. i.e. to deprecate the following usage:

...
const header = <h1>Hi</h1>;
return <Y children={[header]} />;
@esr360

This comment has been minimized.

Copy link

esr360 commented Dec 23, 2019

Regarding the deprecation of defaultProps, I support this in theory, but in practice it is actually very useful. Consider this code:

import styles from '../';
import config from '../';

const MyModule = ({ title, ...props }) => {
  ...
}
  
MyModule.defaultProps = { styles, config }

...without defaultProps, I would have to rename my styles and config imports, unless I'm mistaken, and end up with code that is more verbose, which seems counter-productive.

@remmythical

This comment has been minimized.

Copy link

remmythical commented Dec 23, 2019

I have a question i haven't yet seen answered (please excuse if i've just overlooked it, i wasn't able to read everything), it is perhaps related to the unresolved issue.

In a later major, we'd stop extracting key from props and therefore props is now just passthrough.

If props become passthrough, with key being a new keyword in some way:
Wouldn't that mean that key is now a valid passed prop in React when spread in via {...props}?

Because something like
<div @key="foo" {...props} />
could effectively lead to
<div @key="foo" key="bar" />.

(proposed@key syntax for highlight)

This behaviour (@key keyword vs key prop) might seem logical if the old key syntax didn't already exist.
But I think it's clear that this isn't wanted because it is confusing and might break lots of old code with different assumptions, that wasn't changed in the upgrade stage. (which is of course allowed in a major, but i think it might lead to not immediately visible bugs that are hard to debug)

However, to restrict the use of key as a normal prop, we would still have to dynamically check for a key prop and remove it and issue a warning, like what is described for the upgrade path stage.

However, then we would not fix the described issue:

The problem with this is that we can't statically know if this object is going to pass a key or not. 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 might have overlooked something, but currently i don't see how we could get rid of this dynamic check in the described way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.