Skip to content

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.

@i-am-the-slime
Copy link

Let's go with the canary, it's just a word 😁

@i-am-the-slime
Copy link

@tom-sherman can you release your fork under some name on npm for people like me to try?

@DanielSRS
Copy link

Wouldn't be possible to maintain both? Many projects do this by creating branches for major releases to be able to release patches while the current version lives in main/master.

Even if it's not possible, there hasn't been much activity in recent months so maybe worth merging anyways?

@olegtaranenko
Copy link

React 19 is out :-)

@bondarenkod bondarenkod mentioned this pull request Mar 12, 2025
@SteveKekacs
Copy link

Any update on this?

@sindresorhus
Copy link
Collaborator

@tom-sherman Still interested in working on this? No worries if not. It's been a while.

@Russiee
Copy link

Russiee commented May 9, 2025

Any progress on this? It's the final blocker on migration to React 19 for ourselves and given the PR above and this issue: #688 seems to be a fairly minor fix.

@sindresorhus
Copy link
Collaborator

Closing for lack of activity: #720

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.

8 participants