facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.39k stars 46.38k forks source link

React Fire: Modernizing React DOM #13525

Closed gaearon closed 4 years ago

gaearon commented 6 years ago

For latest status, see an update from June 5th, 2019: https://github.com/facebook/react/issues/13525#issuecomment-499196939


This year, the React team has mostly been focused on fundamental improvements to React.

As this work is getting closer to completion, we're starting to think of what the next major releases of React DOM should look like. There are quite a few known problems, and some of them are hard or impossible to fix without bigger internal changes.

We want to undo past mistakes that caused countless follow-up fixes and created much technical debt. We also want to remove some of the abstraction in the event system which has been virtually untouched since the first days of React, and is a source of much complexity and bundle size.

We're calling this effort "React Fire".

🔥 React Fire

React Fire is an effort to modernize React DOM. Our goal is to make React better aligned with how the DOM works, revisit some controversial past decisions that led to problems, and make React smaller and faster.

We want to ship this set of changes in a future React major release because some of them will unfortunately be breaking. Nevertheless, we think they're worth it. And we have more than 50 thousands components at Facebook to keep us honest about our migration strategy. We can't afford to rewrite product code except a few targeted fixes or automated codemods.

Strategy

There are a few different things that make up our current plan. We might add or remove something but here's the thinking so far:

Tradeoffs

Rollout Plan

At this stage, the project is very exploratory. We don't know for sure if all of the above things will pan out. Because the changes are significant, we will need to dogfood them at Facebook, and try them out in a gradual fashion. This means we'll introduce a feature flag, fork some of the code, and keep it enabled at Facebook for a small group of people. The open source 16.x releases will keep the old behavior, but on master you will be able to run it with the feature flag on.

I plan to work on the project myself for the most part, but I would very much appreciate more discussion and contributions from @nhunzaker, @aweary, @jquense, and @philipp-spiess who have been stellar collaborators and have largely steered React DOM while we were working on Fiber. If there's some area you're particularly interested in, please let me know and we'll work it out.

There are likely things that I missed in this plan. I'm very open to feedback, and I hope this writeup is helpful.

matteing commented 6 years ago

I love this. Reducing bundle size and the "class" prop are changes that will be very welcome.

Great work!

developit commented 6 years ago

🙂

erikras commented 6 years ago

Attention form library authors! 🤣

LaurenceM10 commented 6 years ago

Great!

ryanflorence commented 6 years ago

className → class is fantastic

What about all the others? Seems weird to still be doing clipPath, htmlFor, tabIndex, etc.

diegohaz commented 6 years ago

Adopting class is a major breakthrough in making the library more friendly for beginners. Congratulations.

tannerlinsley commented 6 years ago

This is awesome. I'm so curious how the move to class is actually going work with props.

Seems like ({ class }) => <div class={class} /> would initially present a reserved keyword problem?

solomonhawk commented 6 years ago

This is fantastic news, thanks @gaearon!

felixfbecker commented 6 years ago

I love every of these points, except the className change. It seems downright contradictory to the goal the other points are pursuing (aligning with the DOM API). React binds to DOM properties, not HTML attributes (this is this even articulated in the first point). The DOM Element property is named className, not class. So why would it be named class in React?

alexparish commented 6 years ago

Fantastic! Do you have a goal for bundle size reduction?

mknepprath commented 6 years ago

👏

gaearon commented 6 years ago

What about all the others? Seems weird to still be doing clipPath, htmlFor, tabIndex, etc.

I’m open to discussion but I’d argue these changes aren’t worth it (except for maybe).

nhunzaker commented 6 years ago

I think a re-write of the event system is the most interesting aspect of this. There is significant opportunity to reduce the bundle size and ease community contributions.

Let's do it!

kentcdodds commented 6 years ago

I wonder if it would be worthwhile to also work on releasing JSX 2.0 at the same time? People are going to need to re-learn a handful of things anyway. Maybe it's better to have to re-learn a bunch at one time rather than a few things twice over a period of time? Just a thought that occurred as I read this.

gaearon commented 6 years ago

I love every of these points, except the className change. It seems downright contradictory to the goal the other points are pursuing (aligning with the DOM API). React binds to DOM properties, not HTML attributes (this is this even articulated in the first point).

And yet if we pass an unknown key/value pair it will be treated as an attribute since React 16. So we’re already inconsistent. Also, my comment was about users being wrong in expecting React to set value attribute. Whether React API uses attribute name or property name in its API is compeletely orthogonal.

I’ve defended your side of this argument for years but I think now that this is friction that’s just not worth it. You don’t gain anything from it. Just letting people use class has no negative effects except it doesn’t work with destructuring, and the migration cost. Everybody complains about it when they learn React. I think doing what people expect in this case is more important than being pedantic. And since we’re changing other DOM things anyway, let’s batch this together.

erikras commented 6 years ago

As long as React Fire is blazing fast.... 👍

kentcdodds commented 6 years ago

These changes are all fantastic. I'm way excited about this and its implications for react-testing-library. In particular, events being bound to the react root (or maybe even drop event delegation altogether as it may not be necessary in modern environments anymore?), potentially removing/rewriting synthetic events, and onChange -> onInput will seriously improve the implementation of react-testing-library and the experience in using the tool.

I'd love to provide feedback on this as it's being implemented.

gaearon commented 6 years ago

maybe even drop event delegation altogether as it may not be necessary in modern environments anymore

We considered this but think this might be an oversimplification. Event delegation lets us avoid setting up a bunch of listeners for every node on the initial render. And swapping them on updates. Those aspects shouldn’t be ignored. There is likely more benchmarking to be done there though.

drcmda commented 6 years ago

@tannerlinsley ({ class: className }) => <div class={className} /> unfortunately this will kill jsx 2.0 object short hand notation (<div class />), but so be it ...

It would be super, super nice if class could finally take objects and arrays btw next to strings.

gaearon commented 6 years ago

Do you have a goal for bundle size reduction?

Dropping a third of React DOM would be nice. We’ll see. It’s hard to say early but we’ll do our best.

AlexGalays commented 6 years ago

Wow, this is an enumeration of almost all the design decisions I mention when people ask me about React cons.

Love the direction this is going.

jacobp100 commented 6 years ago

What would the upgrade path be for libraries that use className and want to support multiple versions of React?

ryanflorence commented 6 years ago

@gaearon Maybe it's not a big deal, today its nice to say "props are DOM properties not HTML attributes", now it'll be "except className, that one is the HTML name". I'd also like to be able to copy/paste SVG w/o 10 minutes of messing around with attributes to match up with React's

fforw commented 6 years ago

What about htmlFor?

phpnode commented 6 years ago

It feels like the className -> class transition will have to be done very carefully, probably over an extended period of time. It's going to cause a lot of issues for the ecosystem, as virtually every component will need to be changed - even very trivial ones. This is mostly fine if you "own" the code and there's a codemod, but when you're dependent on 3rd party libraries you're largely at the mercy of maintainers.

The rest of the changes seem relatively low risk from an ecosystem point of view, but getting rid of className will really cause a lot of pain. It seems like it should be possible to split that into a separate issue and release it on a different schedule to the rest of the 🔥 changes.

Pajn commented 6 years ago

I agree with @felixfbecker Everything sounds awesome and would improve quality for both developers and users, but the className change.

Beeing able to deconstruct all properties but one would certainly cause more confusion and be harder to explain to new users than that they need to use className because class is a keyword (which they can clearly see when the misstake is made, as it's colored differently). Working around class in deconstructing requires introducing new syntax or a completely different way to read out that specific property that would only work untill you need to use a rest property. It creates many problems just to save four characters.

caub commented 6 years ago

@felixfbecker it could it be class/for in JSX and className/htmlFor in the JS version?

<label class="foo" for="bar">..</label>

would be

React.createElement('label', {className: 'foo', htmlFor: 'bar'}, '..')
alexeybondarenko commented 6 years ago

Great plan! Nice to here that! 👏👏👏

adeelibr commented 6 years ago

This is awesome, I can't wait to dig into the new stuff.

mizchi commented 6 years ago

Drastically simplify the event system (#4751).

Now react can not delegate handler to custom elements, without extending ReactDOM definition. https://github.com/facebook/react/issues/9242

It means React can not set custom handler on custom element like <x-component onMyEvent={ev => {...}} />

@gaearon Do you have any plan about this?

AlexGalays commented 6 years ago

All these changes are excellent but the biggest, meta change of them all is the size reduction in my opinion. React has a mean DX, but it's fat enough that downloading and parsing it on average networks and in particular mobile is a problem.

We could have it all!

joshduck commented 6 years ago

Would rewriting the event delegation open the door for fixing #1254; where some events degrade perf for the node they are attached to (which for React means the whole app).

Also, as a long shot, have you considered having synthetic dataSet props? Not being able to type allowable HTML props (because of data-*) leads to a ton of bugs when forwarding props to the DOM nodes. Currently typed React components have to choose between exact types for props and allowing data attributes.

NangoliJude commented 6 years ago

I'm excited

jxub commented 6 years ago

@ryanflorence A bit offtop but it's kinda sad that no one (as far as I know of) had the idea to make a html/css/svg -> jsx transfomer in order to ease migrations to React with so many trivial changes to map HTML attrs to React props. So many man-hours wasted perfoming mostly find-and-replace :(

dmitriid commented 6 years ago

It's very strange to see the following in the same issue (and the comments):

Our goal is to make React better aligned with how the DOM works className → class Seems weird to still be doing clipPath, htmlFor, tabIndex, etc.

When all these are direct counterparts of DOM APIs

And this argument doesn't pass muster:

I’ve defended your side of this argument for years but I think now that this is friction that’s just not worth it. You don’t gain anything from it. Just letting people use class has no negative effects except it

React has always been just a very thin layer on top of JS. So everything else besides JSX's angle brackets was JS and had a direct counterpart in DOM APIs for things directly related to DOM: className, clipPath, htmlFor, tabIndex etc. etc. etc.

With the decision to introduce class React stops being a thin layer (class is a reserved word in JS) and breaks away from the declared goal of being more compatible with DOM APIs.

It's especially jarring to see that you want to both "Migrate from onChange to onInput" because it's inconsistent with the DOM and become inconsistent with the DOM by migrating className -> class.

Additionally, this opens a full can of worms, as people will demand (and are already demanding) changes to other parts. Just in the comments: why do we use dataset instead of data-*? Maybe because data-* is not valid JS (much like class) and because it's consistent with the DOM APIs? Why don't we change htmlFor? Because for is a reserved word and htmlFor is in DOM APIs? Etc. etc. etc.

felixfbecker commented 6 years ago

And yet if we pass an unknown key/value pair it will be treated as an attribute since React 16. So we’re already inconsistent.

@gaearon which is also the reason why React is the only library scoring bad on the CustomElements Everywhere interop test: https://custom-elements-everywhere.com/ And there's many issues asking to change it: https://github.com/facebook/react/issues/11347, https://github.com/facebook/react/issues/7249, https://github.com/facebook/react/issues/9230

tomdale commented 6 years ago

This may be a non-issue, but is the similarity of React Fire to React Fiber going to be confusing for non-native English speakers? I've often thought that Newark Penn Station and New York Penn Station being on the same train line here in NYC was a particularly cruel joke on tourists.

If it's a real concern, you could call it React Flame and still keep the fire 🔥 emoji.

gaearon commented 6 years ago

Would rewriting the event delegation open the door for fixing #1254; where some events degrade perf for the node they are attached to (which for React means the whole app).

Fixing #1254 in some form is definitely something I’d like to see.

Also, as a long shot, have you considered having synthetic dataSet props?

I don’t want to commit to something like this now because there’s a bigger surface. Richer interface for DOM (ariaSet, dataSet, classList) makes sense but it’s not clear how much we want to invest in that in React DOM, versus a higher level library that gives you a richer DOM API. Since these changes are more cosmetic but have high surface area, I’d hold off with them.

nickserv commented 6 years ago

React Blazing 🔥

mfix22 commented 6 years ago

@ryanflorence A bit offtop but it's kinda sad that no one (as far as I know of) had the idea to make a html/css/svg -> jsx transfomer in order to ease migrations to React with so many trivial changes to map HTML attrs to React props. So many man-hours wasted perfoming mostly find-and-replace :(

@jxub https://transform.now.sh/html-to-jsx/

gabrielfalcao commented 6 years ago

So excited for the new changes, you humans from Facebook are making history with this migration. 50k components will be migrated over to React Fire ? Your test suites must be so tight <3

lucask42 commented 6 years ago

is the similarity of React Fire to React Fiber going to be confusing for non-native English speakers?

maybe for hard-of-hearing folks, too (or those with other conditions affecting word discrimination)

LucaColonnello commented 6 years ago

Thanks for sharing @gaearon, that’s amasing!

I’d love to see a JSX 2.0 release solving the white space and new lines issue that occurs when we compile our code with Babel and Typescript.

There are different issues opened and I tried to contribute but got told off cause the React team needs to review everything around JSX.

This issue relates to the dom anyway, cause the way we translate jsx to js does not allow to what the w3c says.

This is the issue https://github.com/facebook/jsx/issues/19

My comment is at the very end.

doasync commented 6 years ago

I think, className is ok. Let it be what it is. Don't add insult to injury.

nickserv commented 6 years ago

Can someone please clarify how this affects existing handlers?

Stop reflecting input values in the value attribute

Will React still have controlled inputs with accurate event.target.value updates in handlers, or does this only affect reading values from refs and DOM Nodes?

klarstrup commented 6 years ago

@nickmccurdy it affects what you see in the browser devtools

hoIIer commented 6 years ago

@tomdale React Ember 🔥

denyskotsur commented 6 years ago

Nice! I'm waiting to see full list of changes in React 17. I believe it will be a new era of ReactJS. 🔥⚛️

empyrical commented 6 years ago

@tomdale Hmm: Yarn, Fiber, Fabric; maybe another clothing related term could be used? 😆

gaearon commented 6 years ago

And yet if we pass an unknown key/value pair it will be treated as an attribute since React 16. So we’re already inconsistent.

@gaearon which is also the reason why React is the only library scoring bad on the CustomElements Everywhere interop test: https://custom-elements-everywhere.com/

No, that's not the reason (custom and normal elements are completely separate code paths). The reason is that we already had the old behavior and didn't want to break backwards compat unless the new behavior was solid. I think it would make sense to tackle better custom element interop as part of this umbrella.

is the similarity of React Fire to React Fiber going to be confusing for non-native English speakers?

Both are internal codenames and don’t really carry any significance once the projects are completed. React Fire is just an effort to make React DOM better — and by the time it becomes production-ready it will be just React DOM.

Will React still have controlled inputs with accurate event.target.value updates in handlers, or does this only affect reading values from refs and DOM Nodes?

Yes, because event.target.value is a property. We're talking about stopping updating attributes. Which no other popular libraries do (AFAIK) and which causes countless problems. It shouldn't affect your code unless you're relying on CSS selectors on the value (which is probably bad anyway).

Nice! I'm waiting to see full list of changes in React 17.

Note we're not committing to this being ready by 17. It might be 18. Or 19. 😅