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

Make react-dom and react-art depend on react-reconciler from npm #24979

Closed
devongovett opened this issue Jul 23, 2022 · 29 comments
Closed

Make react-dom and react-art depend on react-reconciler from npm #24979

devongovett opened this issue Jul 23, 2022 · 29 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@devongovett
Copy link
Contributor

Currently, the react-dom and react-art packages each have their own copy of react-reconciler compiled into them. This means, if you use them together, you get two copies bundled in your app. react-reconciler is around ~93 KB minified, or ~30 KB min + gzip, which is not super small.

This also affects other renderers like react-three-fiber, react-pixi, react-pdf, etc. Each of these depends on the react-reconciler package from npm, but react-dom also has a separate copy meaning if you use them you get at least two copies.

It would be nice if react-dom depended on the react-reconciler package from npm rather than compiling an additional copy into the distribution. This would reduce the bundle size of applications using multiple renderers by at least 30 KB min + gzipped.

Is there a reason this is not already the case?

@devongovett devongovett added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jul 23, 2022
@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jul 23, 2022

  1. The inlining is important for runtime perf and some parts of the reconciler isn’t used. Perf is at least way less predictable and likely to cause VM deopts due to polymorphism.

  2. Versioning. The host config protocol isn’t stable. It changes a lot. So as soon as a new version is published everyone would have to be immediately up-to-date.

We want to add some kind of lower level bindings for implementing custom “host components” but even that is kind of still influx since we’re changing the protocol and adding new features that host components can do. However this would still be more targeted to low level operations on a particular environment (eg dom nodes).

It’s actually kind of hard just to preserve the versioning between the “react” package and the reconcilers today. There are breaking changes and not all reconcilers work with the all the minors of “react” for the same major.

This would make that worse, or you’d be duplicating the code anyway.

@sebmarkbage
Copy link
Collaborator

Tbh I’ve been wanting to go the opposite way and have custom reconcilers use the same build tooling we use to inline the config. They run unnecessarily slow and the full react-reconciler’s size is much larger than it would be when inline and dce with a custom host config. Eg hydration code can be dce.

You can tell the cost of the reconciler in the react-art package. More like 23kb gzipped.

@devongovett
Copy link
Contributor Author

Would be curious to see how much runtime perf changes. Wonder how this changes with modern bundlers that perform scope hoisting.

Wouldn't versioning be solved using a pinned version from npm? Or make react-reconciler follow semver. Then at least there is a better chance of them overlapping. Looking at the version history on npm, it doesn't look like it changes more than a few times a year (maybe more recently with React 18 release).

have custom reconcilers use the same build tooling we use to inline the config

I tried this by copy pasting the reconciler package, removing the function wrapper, and assigning the $$$hostConfig variable at the top of the file. It helped some, but not as much as I hoped.

More like 23kb gzipped.

It's 28kb by default, so this isn't that big a difference. Similar difference to what I saw doing the above.

@sebmarkbage
Copy link
Collaborator

At the end of the day though, the DOM one is by far the most important one and the main product. The other use cases tend to be more esoteric and loaded in a specific setting. We wouldn’t compromise the main one that tends to load earlier and is a predictable and cache stable bundle. Especially since it’s susceptible to way more scrutiny.

If anything it would have to be a special build that opts into deopting the DOM build to share it. It likely wouldn’t get as much love and would be ok if it deopted.

What’s your use case?

@devongovett
Copy link
Contributor Author

devongovett commented Jul 23, 2022

Well, ideally it probably wouldn't be a custom reconciler but something else (like you mentioned for implementing custom "host" components), but custom reconcilers exist today so I was trying to come up with something there.

A bunch of libraries have a problem where they need to know about certain types of descendants. For example, a list component with keyboard navigation needs to know what elements exist in the collection in order to implement things like typeahead, arrow keys, selection, etc. Reach UI has a good overview of a bunch of different approaches to this. The most commonly used of them involve rendering all of the items to the DOM, and using some kind of context-based registration system to tell the parent about themselves, and the DOM to sort them into the correct order.

This has the downside that all of the items must be in the DOM at all times. In some cases, like virtualized scrolling, or a combobox/select where users can set the item without showing the list, some or all of the items shouldn't be rendered to the DOM. In React Aria, we walk the JSX tree to do this, which makes for a more natural API than giving up JSX completely (info). But this breaks composition, because only certain known element types are allowed.

With a custom reconciler, I could make <item> a host component, and build my own tree, without rendering any deeper than that until the real items are rendered to the DOM. I did a quick prototype and it seems to work ok, but the additional bundle size required for another copy of react-reconciler means I probably won't go this way.

I'm sure you have ideas on how this could be solved in the future (happy to discuss them here or elsewhere!), but I was looking for something that would work today and this seemed pretty close.

@sebmarkbage
Copy link
Collaborator

There are not yet portals between custom reconcilers and react-dom so even if you had one you’d loose a lot of features and fidelity in the seams. I think you would want the rest of the DOM to keep working like it does in the same reconciler. So it would be more like an intercept layer. One solution to this is just to fork react-dom like a long lived patch and then have users alias it.

Ofc it probably would be better if something in React could do what you needed.

I’m curious what this approach gives you over context registration?

@devongovett
Copy link
Contributor Author

Mainly just that the items don't all need to be in the DOM for it to work. So virtualized scrolling can work, and still support keyboard navigation to items out of view (e.g. Home/End). And beyond that, sometimes we need the collection of items even when none of them are rendered, e.g. a closed combobox/select can still support typeahead (example, try focusing the button and typing, or using the left/right arrow keys), or to render the selected item's value.

@sebmarkbage
Copy link
Collaborator

I think I’m missing something because you don’t need to render them into the dom to register some abstraction with a parent.

You can register a component that doesn’t renders its children for example, which I assume is how you’d do it with the custom renderer.

You still need to render some part of the item if it’s wrapped in abstractions so neither solution works if it’s virtualized at the children level.

@nihgwu
Copy link
Contributor

nihgwu commented Jul 23, 2022

Hey @devongovett, I'm developing a package which can solve the problem you described above, for list you can render the abstraction without deep children to get the registration and then custom your render based on it

@devongovett
Copy link
Contributor Author

I'm not sure if it's possible to get the correct order for registered items without the DOM. You can register items with a context, but if React renders the items out of order for some reason (suspense?), or a new item is added without the parent list re-rendering (e.g. in a child), you won't know the right index to insert it. That's why all the implementations I've seen register the objects plus a ref to an element, and then sort the array based on the rendered DOM order.

@nihgwu
Copy link
Contributor

nihgwu commented Jul 23, 2022

@devongovett If a item is inserted, it will clear registration and rescan, so the order will be guaranteed, same for suspense, as it will insert new items, I built a test page for this case

BTW, actually the package is initially created to make component composition more predictable for layout and easier for a11y support, but I think it would be nice to support list slots as it's the last missing piece 😄

@devongovett
Copy link
Contributor Author

@nihgwu my reply was directed at @sebmarkbage, sorry for the confusion. 😄

But yes, I suppose you could force re-render every item whenever a new one is added/removed. Just seems a bit inefficient.

@nihgwu
Copy link
Contributor

nihgwu commented Jul 23, 2022

@devongovett sorry for interrupting but I guess my idea is a possible solution for your problem, and yes it's seems not that efficient, so I'm thinking if it's possible to support slots in React directly instead of such kind of hack

@Eldeloro1
Copy link

I'm not sure if it's possible to get the correct order for registered items without the DOM. You can register items with a context, but if React renders the items out of order for some reason (suspense?), or a new item is added without the parent list re-rendering (e.g. in a child), you won't know the right index to insert it. That's why all the implementations I've seen register the objects plus a ref to an element, and then sort the array based on the rendered DOM order.

@drcmda
Copy link

drcmda commented Jul 24, 2022

i think i'll forward some twitter exchange here, in 2020 dan suggested that we inline the reconciler for react-three-fiber. this was not possible back then and we had to revert.

https://twitter.com/0xca0a/status/1318138809637392386
and the thread explaining why and us trying it https://twitter.com/0xca0a/status/1318138823142944769

the take away was that react is forced to remain cjs because only cjs can have runtime require (deciding between dev or prod). a library that has tree-shakable exports needs to be esm, and esm can't require. now to fix this with package.json export resolution turned out to be so complex and buggy that it basically made no sense back then. perhaps this has changed i don't know but according to Mateusz Burzyński it seemed bleak:

We’ve tried to add support for pkg.exports in preconstruct (library packager like tsdx, microbundle, etc) and there are, for now, too many situations which are too complex too handle for us. Especially if u consider that a packaged package can have deps

so it's either cjs with good DX but it won't tree shake.

or reconciler inlined esm with DX like this, "Error at Zb" for adding a component that doesn't exist, a typo, etc.

Screenshot 2022-07-24 at 09 44 37

@sebmarkbage
Copy link
Collaborator

Sooner or later we’ll go for ESM in React and then bundlers will need to be ready for package.exports. My guess right now is that we’ll continue with DEV mode by default. The rationale is that if you’ve written a product with dev mode then it’s a one line change to enable production mode, but going in the reverse order you might have to do significant rewrites to fix all the warning before you can enable dev mode. However this means that a bundler that doesn’t support the “production” condition will run dev mode in production.

So this will become very urgent very quickly once we do make this move in React.

@devongovett
Copy link
Contributor Author

Found an interesting alternative to a custom reconciler: a fake DOM. By implementing the very basics of the Element and Document APIs (e.g. createElement, appendChild, removeChild, setAttribute, etc.), and rendering into it using a portal, I can build up a custom tree without rendering to the real DOM. An advantage to this is that it also works with context.

I still think it would be nice to fix the bundle size issue with custom reconcilers though.

@souporserious
Copy link

@devongovett would love to chat about the fake DOM solution further, is it possible for others to test yet? I spent the last few weeks working on Reforest which aims to do something similar so things like context work as normal. My solution is pretty hacky though since it relies on proxy state through valtio and hacking Suspense to wait on the server before committing the final render.

@sebmarkbage
Copy link
Collaborator

A downside of the fake DOM is that you ofc break the inline caches and now create polymorphic APIs at every interaction with the DOM - deopting everything else on the page (slightly).

@nihgwu
Copy link
Contributor

nihgwu commented Aug 4, 2022

Don't understand why we need a fake DOM to make context work, in my solution, I simply double render the element while for the first time I only gather the props but render nothing which is super cheap and then use the collected props with real component, so it works with all the features React offers

@sebmarkbage
Copy link
Collaborator

Fake DOM might help since React will likely add focusing capabilities to its Scope that you would want to find in a virtualized list if you try to get focus.

@souporserious
Copy link

souporserious commented Aug 4, 2022

It seems like the call-return API that was previously proposed would work with Suspense now? That API seems to elegantly solve all of these issues of keeping track of indexes and getting/injecting data from children while retaining composition. Also, sorry I know this thread was originally meant for the reconciler, I'm happy to start a different issue around this discussion.

@nihgwu
Copy link
Contributor

nihgwu commented Aug 4, 2022

yes I think react-call-return would make slots pattern officially supported in React, my solution works very similar to it but not very elegant on implementation, in my package it's called createHost - createSlot vs createCall - createReturn in react-call-return, it's really useful to compose component in a configurable way and super powerful to support A11y in UI library

@nihgwu
Copy link
Contributor

nihgwu commented Aug 4, 2022

Anyway I'll try to create a proposal to https://github.com/reactjs/rfcs to add Slots support in React, then we can discuss there

@devongovett
Copy link
Contributor Author

With a context-based solution, how would you ensure that the order of the tree you construct is correct? When a new item is inserted, you don't know what position to insert it. So you'd have to force re-render the entire collection and re-collect the whole tree. Or wrap each child in a context provider with the index like in @souporserious solution, but then you can't have a child that returns multiple items.

With a fake DOM, react's reconciler does all the work to handle minimal updates, and it tells me where to insert new items (via insertChild). It also feels cleaner IMO because React is literally designed to build and reconcile trees. Short of an API to walk the fiber tree offered by React, this is the cleanest version I've found so far that doesn't require rendering all items to the real DOM.

@nihgwu
Copy link
Contributor

nihgwu commented Aug 5, 2022

It also feels cleaner IMO because React is literally designed to build and reconcile trees. Short of an API to walk the fiber tree offered by React

Can't agree more, that's why I think it would be nice to have the api built in React, like react-call-return

I've seen different approaches on slots(or parent-children communication) support for React, double rendering is the easiest way I've found to work in all occasions and full featured with the current React api, it also doesn't require rendering all items to real DOM.

But yes if it's(call-return) not going to be supported in React, use a custom renderer with react-reconciler would be a nicer solution

@nihgwu
Copy link
Contributor

nihgwu commented Aug 5, 2022

Created a RFC reactjs/rfcs#223

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

6 participants