search

fable-compiler / Fable.Lit

Write Fable Elmish apps with Lit
https://fable.io/Fable.Lit/
MIT License
91 stars 13 forks source link

Should we delete the "custom HMR" ? #25

Closed MangelMaxime closed 2 years ago

MangelMaxime commented 2 years ago

Hello @alfonsogarciacaro,

someone had an issue with how the namespaces/modules are organized in Fable.Lit.Elmish (see https://github.com/elmish/hmr/issues/33).

His issue, also made me think about the fact that this is confusing to have an HMR implementation in this package and another one advertise "more global" to Elmish in Fable.Elmish.HMR.

Now that Fable.Elmish.HMR supports Parcel, Webpack and Vite (https://github.com/elmish/hmr/issues/29) what do you think of removing the copy/paste code from this repository?

From my quick tests it seems like Fable.Elmish.HMR works with this package too as this is just an Elmish application in the end?

If we are to do that, I suppose we need split Fable.Elmish.HMR into smaller modules to avoid having Fable.React being introduced and so femto asking to ask to install react and react-dom.

alfonsogarciacaro commented 2 years ago

Yes, indeed it would be ideal to have a common Elmish.HMR (Fable.Lit will still keep some specific HMR code but that's unrelated to Elmish).

The main issue, as you said, are Elmish.HMR dependencies, particularly Fable.React and to a lesser extent Elmish.Browser (though it'd be nice to remove the dependency if we had termination capabilities). We can break Elmish.HMR into Elmish.HMR/Elmish.HMR.React but this means React users will have to install an extra package to make it work. Another possibility would be to add this code directly to Elmish.React with #if DEBUG as it only checks a global variable so it doesn't even need a dependency on Elmish.HMR.

MangelMaxime commented 2 years ago

Yes, indeed it would be ideal to have a common Elmish.HMR (Fable.Lit will still keep some specific HMR code but that's unrelated to Elmish).

That's fine to me as this is specific to Fable.Lit

Elmish.Browser (though it'd be nice to remove the dependency if we had termination capabilities)

I think that when we have termination capabilities it should be able to remove the old listener.

Another possibility would be to add this code directly to Elmish.React with #if DEBUG as it only checks a global variable so it doesn't even need a dependency on Elmish.HMR.

This code also checks if there is the HMR modules presents.

In the past Eugene said he didn't want to have any code specific to HMR in the main modules. I think this is both to keep the code simple and also because he can't assure to be able to maintain it.

I am only able to maintain HMR because I invested a lot of time initially in it and even today it requires a lot of works to make it works correctly.

We can try to ask Eugene opinion on it today.

The ideal scenario would be:

  1. Having Fable.Elmish.Browser use termination feature to clean itself
  2. Fable.Elmish.React check for the global variable and wrap the HMR code inside a #if DEBUG directive
  3. Make Fable.Elmish.HMR dependant on Fable.Elmish elmish allowing it be used in Fable.Lit.Elmish, Fable.Snabbom.Elmish, Fable.Elmish
alfonsogarciacaro commented 2 years ago

I removed HMR from Lit.Elmish 1.4. We'll live with the React dependency for now :)