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

[WIP] Make UseElmish compatible with Fast Refresh #423

Closed

Conversation

alfonsogarciacaro
Copy link
Contributor

Thanks for merging #414 @Zaid-Ajaj! I'm trying now to make the useElmish hook compatible with HMR, more specifically with the Fast Refresh plugin. I think I've made it work (I'm testing this with Vite and Vite's React plugin) but there are some notes:

  • The only way I could make it work was to inline the function in Debug mode so the plugin could "see" the useState hook. Feliz Hook Fable plugin (which changes the name of the import so it matches hook standards) didn't seem to have an effect. For the same reason, I had to use the hook bindings from Fable.React instead of the equivalents from Feliz. (If I'm not doing things wrong and Feliz hooks are indeed not compatible with Fast Refresh you may need to consider inlining them too).

  • It seems the effects on connection/disconnection are run during HMR, so we need to be careful not to reset the state.

Again, I'm sending the WIP PR to start a discussion. If the approach looks good I can complete it 👍

cc @MangelMaxime

@Zaid-Ajaj
Copy link
Owner

Hi @alfonsogarciacaro, this is a bit hard for me to debug at the moment, a bit low on time these days to dig into these kinds of issue so I hope you don't mind if I keep the PR open for a bit. However, it is interesting to hear that the hooks aren't being picked up by fast-refresh 🤔

so the plugin could "see" the useState hook. Feliz Hook Fable plugin (which changes the name of the import so it matches hook standards) didn't seem to have an effect.

Using inline hooks shouldn't be necessary because fast-refresh is also able to pick custom hooks built with useEffect / useState under the hood. The thing is, fast-refresh is very picky about what code is compiled in a single page:

  • React components should be exported upper-case functions
  • Hooks call-sites should start with use* (hence why [<Hook>] is needed)
  • All other code in the same file should not be exported

I think usually the third point is the culprit of fast-refresh not working with useElmish because we define init and update in the same file which is not allowed* according to fast-refresh requirements

  • last time I checked these requirements was with fast-refresh 0.4.x however fast-refresh is now sitting at 0.5.0 so things might have improved

@MangelMaxime
Copy link
Contributor

I think usually the third point is the culprit of fast-refresh not working with useElmish because we define init and update in the same file which is not allowed* according to fast-refresh requirements

If I remember correctly, fast-refresh is looking at what is exposed from the module so if you make the init and update function private with all the types etc. It should be able to pick it.

I didn't re-read the fast-refresh documentation since a long time so I can be wrong.

@Zaid-Ajaj
Copy link
Owner

PR has become stale, @alfonsogarciacaro does the issue still stands?

@alfonsogarciacaro
Copy link
Contributor Author

It's been a while since I haven't tried, sorry. I think I did try by setting everything private except for the component but React Refresh still didn't work until I inlined the custom hooks. Though may be I was doing something else wrong.

Anyways we can close or leave this PR as is and revisit if users cannot make React Refresh work with useElmish.

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

3 participants