search

elmish / react

Elmish React extensions for writing SPA and Native apps
https://elmish.github.io/react
Other
104 stars 21 forks source link

Support React 18 #60

Closed kerams closed 2 years ago

kerams commented 2 years ago

Been using this version of withReactSynchronous for a few days, works fine. However, you do get a warning when HMR triggers

Warning: You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it.

I don't know the HMR mechanism, so no idea how to fix that - the warning appears benign though.

Thanks to the version check backwards compatibility is preserved.

@forki, if this doesn't work for your SSR project, can you please create a minimal sample repo for me?

et1975 commented 2 years ago

As long as we're doing bindings update, are there new RN bindings as well?

kerams commented 2 years ago

https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html

Note for React Native users: React 18 will ship in a future version of React Native.

MangelMaxime commented 2 years ago

About the HMR warning, I think this is because when it triggers we rebuild everything from the beginning.

Meaning that we re-execute the code which initialize the React view. I don't know if there is a way to destroy the preview React application if it exist.

The other solution would be to use a top level react component with an Elmish hooks. But the exisiting published libraries that provide Elmish via hooks are not implemented correctly. I have a fix in one of my project, but I need to take time to test more and propose a PR.

kerams commented 2 years ago

Maybe hold on to the root instance in elmish-hmr? It would mean that repo too would need to know about React versions and internals, ugh.

MangelMaxime commented 2 years ago

Looking at https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html

There is a mention of root.unmount(); which I suppose is removing the React root and would probably allows to call createRoot a second time without the warning.

The question is how to save the root instance to access it later.

MangelMaxime commented 2 years ago

With Elmish 4, I think we would be able to use the termination feature in order to kill the Elmish app properly.

But in Elmish 3, we don't have such features.

kerams commented 2 years ago

Not in production, but I've been using this in dev.

It might be a good idea to mention this warning in the release notes as a known problem, and that it can be safely ignored until we figure it out.

kerams commented 2 years ago

@forki, can you please check this?

Note that unlike createRoot, hydrateRoot accepts the initial JSX as the second argument. This is because the initial client render is special and needs to match with the server tree.

I made the first setState call hydrateRoot, but I'm not sure if the "initial client render matches with the server tree". Subsequent updates just call render on the root.

forki commented 2 years ago

sure with an alpha version or something

et1975 commented 2 years ago

Thanks folks, released as v4.0.0-beta-2

kerams commented 2 years ago

Thank you, after resolving a couple of (strange) dependency issues, I managed to update to the beta. withReactSynchronous works smoothly except for that HMR warning.

forki commented 2 years ago

image

some weird error after upgrading all elmish packages

kerams commented 2 years ago

That's Elmish.Browser, isn't it. It's been removed as a dependency from the HMR package, so I don't have it in my project.

forki commented 2 years ago

yes I assume it is. So what do we do about this?

et1975 commented 2 years ago

It's in urlParser now, here's the line: https://github.com/elmish/urlParser/blob/master/src/parser.fs#L342 I'm not sure how it's getting into this situation, maybe you could look at the values in the debugger, Steffen?

MangelMaxime commented 2 years ago

@et1975 I think urlParser is not yet used stable version of Elmish (v3).

urlParser is introduced when Elmish 4 will be released.

et1975 commented 2 years ago

Don't use v3 browser (or anything, really, that depends on v3 elmish) with v4 elmish, there's a reason it's a major bump... in most cases it shouldn't even compile.

MangelMaxime commented 2 years ago

I checked the code of Elmish.Browser v3:

And in theory all the SubString calls are secured behind a .Length check.

https://github.com/elmish/browser/blob/e4af0c9393ec16161102eef52098fc2b8fd8329f/src/parser.fs#L354-L356

https://github.com/elmish/browser/blob/e4af0c9393ec16161102eef52098fc2b8fd8329f/src/parser.fs#L332-L333