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

React 19 & concurrent rendering #656

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tom-sherman
Copy link

@tom-sherman tom-sherman commented Apr 11, 2024

It's a very ambitious title, I know...

The goal is to support latest canaries and concurrent rendering. If we ever get as far as this being working, it'll need to land in a new major version of ink and it'll be blocked by React 19 (unless we wanna publish an ink version that depends on a react canary version).

I think this also unlocks a more responsive Ink. Spun that out into a separate discussion here: #657

@tom-sherman
Copy link
Author

tom-sherman commented Apr 12, 2024

I have a way forward with the tests. It involves wrapping rendering in act(), unmounting all instances at the end of tests, and moving some tests to async.

@tom-sherman
Copy link
Author

tom-sherman commented Apr 15, 2024

@vadimdemedes @sindresorhus refactoring all of the tests is going to take substantial effort and I'm aware that there's no specific issue for this work. I just wanted to check in with you as to whether this change is something you're in favour of before I invest time into refactoring the tests.

I think this is going to see pretty much every test touched, and so definitely requires some consensus and coordination.

@tom-sherman
Copy link
Author

It would also be good to match the react-dom and react-three-fiber APIs of createRoot(container).render(). This would allow simplifying the API a bit eg. No need for options to be a NodeJS.WriteStream | RenderOptions union anymore as the stream would be passed into the container. We can also get rid of the instances WeakMap this way.

To summarise my proposal:

const root = createRoot({ stdout, stdin });
root.render(<App />);`

@sindresorhus
Copy link
Collaborator

I think this is a good idea in general, but the final decision is up to @vadimdemedes.

@sindresorhus
Copy link
Collaborator

I think it also may be a bit too early. We cannot merge this until React 19 is out as otherwise we are not able to do new releases, and if this stays open for a long time with so many tests changes, it will be prone to lots of merge conflicts.

@tom-sherman
Copy link
Author

tom-sherman commented Apr 16, 2024

That's a good point, I wonder if there is a different way forward.

For example a new createRoot API could be shipped in a minor version (or @next tag) that uses a vendored React canary version - Next.js does this.

I think there's lots of value around allowing users (particularly library authors) to opt into this change before React 19 is released.

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

Successfully merging this pull request may close these issues.

None yet

2 participants