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

POJO Elements #37

Closed
wants to merge 1 commit into from
Closed

POJO Elements #37

wants to merge 1 commit into from

Conversation

iddan
Copy link

@iddan iddan commented Mar 29, 2018

JSX will be transpiled to object literals, native types to factory functions.

@facebook-github-bot
Copy link
Collaborator

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Collaborator

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!


### Security

Unlike in previous discussion this proposal is secure because **type is always a function**.
Copy link

@streamich streamich Mar 29, 2018

Choose a reason for hiding this comment

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

What about custom elements, how will they be transpiled?

<my-custom-element />

Copy link
Author

Choose a reason for hiding this comment

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

Should be added to questions. Any ideas?

props: {
children: "Hello, World!"
}
}

Choose a reason for hiding this comment

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

Why not use JSON-ML:

['div', {}, 'Hello, World!']

Copy link
Author

Choose a reason for hiding this comment

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

Readability and consistency, but totally an option.

Copy link

@yordis yordis Mar 29, 2018

Choose a reason for hiding this comment

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

@streamich what is that JSON-ML? His example is just JSON, what are the benefits of JSON-ML?

Copy link
Author

Choose a reason for hiding this comment

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

  1. It’s a more compact in code and memory format
  2. It’s a standard for describing XML like data structures in JSON


# Motivation

- No more JSX pragma: React-like libraries like Preact would be able to consume React components with **no synthetic syntax modifications**. Of course there can be many other differences but there is enough common ground that worths the changes in my opinion.

Choose a reason for hiding this comment

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

To make other libraries consume JSX, like

<div>Hello world</div>

one could make JSX transpiler emit closures with hyperscript function h dependecy injected:

(h) => h('div', {}, 'Hello world');


# Motivation

- No more JSX pragma: React-like libraries like Preact would be able to consume React components with **no synthetic syntax modifications**. Of course there can be many other differences but there is enough common ground that worths the changes in my opinion.
Copy link

Choose a reason for hiding this comment

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

it seems like you wouldn't avoid the pragma, since the compiled-to type ReactDOM.div is library specific. Wouldn't this have the same coupling as createElement?

Copy link
Author

Choose a reason for hiding this comment

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

Well no, because anything not interacting directly with DOM elements or allows providing replacement components will benefit from this change. Anyway Preact uses class and React uses className and there are other small differences but many components will still be able to target both

Unlike in previous discussion this proposal is secure because **type is always a function**.
Unlike strings a function can't be injected by XSS or similar. If a JSON will be parsed type will remain string and will be considered an error:
```json
{ "type": "div", "props": { "children": "Hello, World!" } }
Copy link

Choose a reason for hiding this comment

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

how will a consumer of the element be able to determine if the type is a host type or a composite type? At the moment one can do isHost = typeof element.type === 'string'

Copy link
Author

Choose a reason for hiding this comment

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

isHost = element.type instanceof ReactDOM.Element

@dantman
Copy link

dantman commented Mar 29, 2018

Currently one of createElement's jobs is to emit dev mode warnings when a ref or key is invalid. To my knowledge this is done in a function call instead of afterwards based on the POJO because afterwards we don't know where the source of the error was and it makes debugging harder. facebook/react#3228

It also freezes things, adds _self, _source, adds warning getters for ref/key, etc... to help with debugging. And it handles defaultProps too.
https://github.com/facebook/react/blob/master/packages/react/src/ReactElement.js#L110-L257

@dantman
Copy link

dantman commented Mar 29, 2018

This will also hinder minification:

The props, children, and types keys can never be optimized away and make the code larger when minified. And JSX is used a lot in general making space wasting syntax in the JSX transform output grow exponentially.


# Motivation

- No more JSX pragma: React-like libraries like Preact would be able to consume React components with **no synthetic syntax modifications**. Of course there can be many other differences but there is enough common ground that worths the changes in my opinion.
Copy link

@thysultan thysultan Mar 29, 2018

Choose a reason for hiding this comment

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

From a performance and compostability perspective, the current semantics allows react and react-like libs to have more control of the resulting data-structure created between patch/minor sem-versions, this would go against that, forcing them to conform to consume what ever data-structure is agreed upon within the reconcile phase.

When considering that these libs add meta data to the snapshot data-structures, this would effectively kill the monomorphic nature that one has control over with the current pragma composition pattern since there would still be a need to add lib specific meta data. i.e Inferno emits it's own meta data for performance reasons, Vue emits it's own meta data as does DIO and i'm sure Mithril, Preact and other reactive libs that emit a primitive snapshot structure include specific meta data for their needs in addition to the normalization each libs undertakes to flatten children.

Copy link
Author

Choose a reason for hiding this comment

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

I see React as more than just a library but an ecosystem and reference for other libraries. This proposal suggests a trade off a custom element instanciation for standard data structure to represent the virtual DOM

@j-f1
Copy link

j-f1 commented Mar 29, 2018

Comparison:

// numbers are how many characters shorter
a.createElement("div",{},"Hello, World!")//34 (4 ch)
a("div",{},"Hello, World!")//3456789012345678 (18ch / 14ch)
{type:a.div,props:{children:"Hello, World!"}}

@iddan
Copy link
Author

iddan commented Mar 29, 2018

If following @streamich proposal JSON ML syntax would actually reduce compiles code size.

@streamich
Copy link

@trueadm
Copy link
Member

trueadm commented Mar 29, 2018

There's a few things I want to point out here:

  • As others have already mentioned, we use React.createElement() to do far more than just create "virtual nodes", they are used for dev-time related checks and validation.
  • We also handle things like "key" and "ref" that are special cases and require runtime logic to make this work. Take for example <MyComponent {...stuff} />, if stuff contains key or ref, we need to extract those from the props – something we can't do statically if we don't know if either property exists.
  • We use a $$typeof property on ReactElement to understand what type of element it is, for this we use an internal system of symbols to ensure we're dealing with something that was created via React. We'd still need this system, we can't simply use the type property here because it would get far too overloaded.
  • In terms of performance, I've found that using a lightweight monomorphic function to create a monomorphic resulted in better performance than creating polymorphic inline objects – because as already mentioned, the hidden classes won't match if you adopt the proposal. To ensure the objects are monomorphic, we'd need to ensure that both ref and key are added to every ReactElement POJO, even if they are not used (they would be set to null).
  • There's far too much focus on JSX -> virtual DOM here. That might be what JSX de-sugars to now, but in the future I expect we'll make compile time optimizations in React and move away from virtual DOM entirely.

I'm not too sure this RFC brings any benefits to React or its ecosystem.

@streamich
Copy link

...and move away from virtual DOM entirely.

Interesting.


### Security

Unlike in previous discussion this proposal is secure because **type is always a function**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps I'm misunderstanding this statement, but type isn't always a function. For example:

  • React.forwardRef, Context.Provider, and Context.Consumer are objects
  • React.StrictMode and React.AsyncMode are symbols
  • Many React Native host types (e.g. View) are strings

This proposal seems very ReactDOM-centric.

Copy link
Author

Choose a reason for hiding this comment

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

The problem is only with bare strings. So as long as type is an object, symbol or function there is no problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Many React Native host types (e.g. View) are strings

props: {
children: "Hello, World!"
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens to key and ref attributes? Although not relevant to this specific example, these are important. Moving them into props would be a pretty big backwards breaking change (because of e.g. {...props}).

Copy link
Author

Choose a reason for hiding this comment

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

They are practically props aren't they today? What mechanisms differentiate them?

Copy link
Collaborator

@bvaughn bvaughn May 7, 2018

Choose a reason for hiding this comment

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

They aren't props. They're stored separately on ReactElement. If they were in the props argument, then spreading props (e.g. <Foo bar="abc" {...props} />) would pass ref and key too. Particularly with refs, this could cause a problem, as it could result in the same ref callback being called for many types of element.

Edit Just noticed @trueadm's comment also touches on this.

Copy link

Choose a reason for hiding this comment

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

Current behaviour:

const React = require('React');
const el = <div b='c' ref={() => {}} key='key' />;
el.props.b; // 'c'
el.props.ref; // undefined
el.props.key; // undefined

@iddan
Copy link
Author

iddan commented May 7, 2018

@trueadm I don't think this proposal has anything to do with the virtual DOM actually. This is a proposal to represent JSX in a more generic and native JS way.

@sebmarkbage
Copy link
Collaborator

I think any proposal like this would like want an intermediate upgrade path in the ecosystem using something like #107 regardless.

After that the question is basically if it should be a function call or inline. All our testing suggest that a function call is the way to go in this case because it allows the VMs to dedupe a lot of the code gen that happens for each of these callsites. So it'd probably end up being a function for that reason. It could in theory be a built-in function in the transpiler runtime rather than on React though. That's covered in #107 too though.

@iddan
Copy link
Author

iddan commented Mar 2, 2019

@sebmarkbage Can you elaborate

All our testing suggest that a function call is the way to go in this case because it allows the VMs to dedupe a lot of the code gen that happens for each of these callsites. So it'd probably end up being a function for that reason.

@iddan iddan closed this Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.