Zaid-Ajaj / Feliz

A fresh retake of the React API in Fable and a collection of high-quality components to build React applications in F#, optimized for happiness
https://zaid-ajaj.github.io/Feliz/
MIT License
534 stars 78 forks source link

Feliz.UseElmish - Update is not called from React.useEffectOnce #540

Closed Dzoukr closed 1 year ago

Dzoukr commented 1 year ago

Hello,

as discussed in #535, here is the repo with the bug description:

Calling React.useEffectOnce in the component successfully logs the message, but calling dispatch will not run the update function

Example repository: https://github.com/Dzoukr/FelizUseElmishIssue

Zaid-Ajaj commented 1 year ago

HI @Dzoukr I was able to reproduce the issue and it was a tricky one because the dispatch function reference is not stable. Only after the Elmish program has started the dispatch-loop, you can dispatch messages that cause a re-render but before that the dispatch you get is a no-op dispatch with this implementation fun (msg: Msg) -> ()

I've implemented a fix for this but it feels a bit hacky: instead of being a no-op, the initial dispatch now keeps track of messages that are dispatched before the program subscribed and dispatches for real them once the dispatch-loop has actually started.

In any case: Feliz.UseElmish v2.2.0 is published. Please take it for a spin and let me know how it goes 😄

@alfonsogarciacaro Your input on this fix would be greatly appreciated 🙏

Dzoukr commented 1 year ago

Hi @Zaid-Ajaj, yup, works great, thanks!

I'd love to know @alfonsogarciacaro opinion on this because Fable.React.UseElmish has the same issue so it seems that it may be actually intended (new React hooks behavior?).

Dzoukr commented 1 year ago

Well, this seems to trigger other problems :/

My basic template fails after paket update of Feliz.UseElmish

Steps:

> mkdir MyTest
> dotnet new install SAFEr.Template::3.0.0-pre-004
> dotnet new SAFEr
> dotnet tool restore
> dotnet run

goto: http://localhost:8080/

Now it works, so let's update to the latest lib:

> dotnet paket update
> dotnet run

goto: http://localhost:8080/

Now the browser is dead with error (after a long time) in console:

react-dom.development.js:12056 Uncaught RangeError: Invalid array length
    at Function.from (<anonymous>)
    at RingBuffer$1__doubleSize (ring.fs.js:83:18)
    at RingBuffer$1__Push_2B595 (ring.fs.js:67:45)
    at dispatch (program.fs.js:164:13)
    at Program$4.setState (UseElmish.fs.js:56:29)
    at processMsgs (program.fs.js:188:25)
    at ProgramModule_runWithDispatch (program.fs.js:204:5)
    at ProgramModule_runWith (program.fs.js:209:5)
    at Util_ElmishState$3.subscribe (UseElmish.fs.js:47:13)
    at subscribeToStore (react-dom.development.js:16958:10)
Dzoukr commented 1 year ago

I don't see the Reopen button - should I create a new issue?

MangelMaxime commented 1 year ago

@Dzoukr I think only maintainers of a project can re-open an issue.

For now, I would wait for @Zaid-Ajaj input so he can fix the issue again or re-open it depending on what he can/prefers to do :)

Dzoukr commented 1 year ago

You are correct, let's wait 😄

Zaid-Ajaj commented 1 year ago

I'd love to know @alfonsogarciacaro opinion on this because Fable.React.UseElmish has the same issue so it seems that it may be actually intended (new React hooks behavior?).

I haven't tested it but I believe this behaviour will also manifest in older React versions < v18 because it's not about useEffect, it is the unstable dispatch that you get from useElmish hook

Well, this seems to trigger other problems :/ [...] Now the browser is dead with error (after a long time) in console

@Dzoukr this error is now coming from the core elmish library here

should I create a new issue?

I will reopen and look into it today

Zaid-Ajaj commented 1 year ago

@Dzoukr Turns out I had the fix in two places and it only needed to be in one place. Published Feliz.UseElmish v2.3.0 that should now work just fine.

@MangelMaxime I see that HMR these days is working fine without needing the hook to start with use? Is that by chance because if not, then I should bring back the [<Hook>] attribute to useElmish

Dzoukr commented 1 year ago

Look good to me, thanks again! ❤️ 🙏

MangelMaxime commented 1 year ago

@Zaid-Ajaj

Hum, I don't know I think for hooks you need to have they starting with use indeed unless they changed something in react-refresh.

If your F# function/method is already prefixed with use then [<Hook>] is optional I think.