search

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
531 stars 77 forks source link

Making UseElmish's dispatch function stable. #587

Closed lukaszkrzywizna closed 7 months ago

lukaszkrzywizna commented 8 months ago

Resolves #588

MangelMaxime commented 8 months ago

Hello @lukaszkrzywizna,

Thanks for the PR

Did you test if your changes support HMR? I am asking because this is something important to keep supporting.

lukaszkrzywizna commented 8 months ago

No problem @MangelMaxime.

I've tested it using the Vite + React Fast Refresh plugin. I've checked if during updating components:

Is that it? Should I check something else?

I've just reviewed changes for #540, and I've noticed that after removing queuedMessages, React.useEffectOnce still works. However, I've decided to not change it for more safety.

MangelMaxime commented 8 months ago
  • the state persist

If the state persist and the application is not broken then that's good enough in general.

  • Vite doesn't notify about HMR problems.

I believe in the browser console you have a log. But in general, if you don't have a full refresh of the page it means that HMR happened.

lukaszkrzywizna commented 8 months ago

@Zaid-Ajaj Did you have a change to look at this?

Zaid-Ajaj commented 8 months ago

@lukaszkrzywizna the changes look reasonable, I haven't had the chance to actually try them on a project though and the problem is I believe we have done this fix already a few times and every time something changes in tooling like vite or fast refresh and we need another fix.

If you say this works with latest tooling, we can get this merged and publish a package ✅ if we hear it breaks things we can revert. Hard to unit test things like this 😓

Zaid-Ajaj commented 7 months ago

Thanks @lukaszkrzywizna for the PR 🙏 ❤️ merged and published as of Feliz.UseElmish v2.5.0 🚀

lukaszkrzywizna commented 7 months ago

If you say this works with latest tooling, we can get this merged and publish a package ✅ if we hear it breaks things we can revert. Hard to unit test things like this 😓

Sorry for the late response. I've been using it in a production for about 3 weeks. No problems so far 🤞

Thanks @lukaszkrzywizna for the PR 🙏 ❤️ merged and published as of Feliz.UseElmish v2.5.0 🚀

Thank you for merging it! I'm glad I could add something to this awesome library 😄