Use Elmish.Program in useElmish #414
Conversation
Feliz.UseElmish/UseElmish.fs
Outdated
| let rec dispatch (msg: 'Msg) = | ||
| promise { | ||
| let mutable nextMsg = Some msg | ||
| static member useElmish(program: unit -> Program<unit, 'State, 'Msg, unit>) = |
There was a problem hiding this comment.
Can we avoid forcing the 'arg type to unit?
Some program can use another parameter than unit for their initial arguments.
Elmish Program definition: type Program<'arg, 'model, 'msg, 'view>
For the view, I suppose you made it unit because we are inside of a hooks and so don't have a view to render in this context?
There was a problem hiding this comment.
Ah, yes. That makes sense, I was mainly thinking in the case when you pass the init and update function, but it should be no problem to accept other args than unit.
About it he view, yes. The view code comes after the hooks, so it doesn't make sense at this point and this is why it's set to unit. Incidentally, this should also make it easier to reuse Elmish programs with different renderers like React or Lit.
There was a problem hiding this comment.
@MangelMaxime I've made the changes to accept an arg other than unit. You're right this is probably useful to have an init function that depends on props passed to the component.
About view, it would be possible to have an overload useElmishthat accepts a Program with a view function (or alsouseElmish(init, update, view)) and returns directly a ReactElement. However when designing something similar for useLit` @Zaid-Ajaj commented that a helper returning ReactElement is not actually a hook, so not sure if it can create an issue with React tooling.
|
I've been out of the loop on this one. Not I totally understand what the underlying issue was in fable-promise 😅 Regardless of that iusse, I think it is a good idea to use the existing implementation of Elmish inside of However, I am not sold on accepting a full
That would be great! |
The view inside of Elmish program is a slot reserved for program that need to have Elmish handle the view. You can use Elmish without any view this is for example what I do in a CLI application where I use Elmish only for the state management. Allowing the hook to accept a program as an entry is important to allow people to augment their Elmish program like it is done for HMR, Navigation, debugger, etc. Because we are using methods, to define the The second version would just call the first overload to make it transparent to the user. |
In practice I am a bit concerned about their use cases. Usually users augment their top-level program with things that are application-wide such as Navigation and HMR but when using What happens when multiple React components with
Yeah an overload would make sense |
For HMR, it would not work indeed because I am using a fixed name global variable. But this should be fixable. But in their case, it was more to attach the Redux debugger for example. I just gave a list of publicly existing program augmentation as an example. I think offering it as an overload for expert user makes sense as right now if they need to access it they can't and have to copy/paste and adapt the source from Feliz repository. |
Sounds like a good idea then. Let's get this in ✅ |
|
I tried to add the "reset" when dependencies change but the test is failing. Not entirely sure why, I'll try to investigate. BTW, it'd be nice to move already tests to Fable 3 and use some better tooling. Right now tests take a long time to run, there's no watch mode and I cannot see which assertion is failing, so it's difficult to spot the problem. |
alfonsogarciacaro
force-pushed
the
elmish-program
branch
from
133e997
to
320a749
Compare
alfonsogarciacaro
changed the title
|
Ok, tests are passing now, I was using upper case for one the test ids 😅 But it took me a while to realize that, I had to create a different project for it. So it would still be nice to update the tests to Fable 3 and enable a test watcher. I can do that in a different PR if that's ok with you. This PR is now ready to merge btw 👍 |
|
@Zaid-Ajaj Would it be possible to review the PR when you've a moment and merge if everything's ok? Thanks a lot! Note: if for some reason you cannot maintain this package anymore, we could publish |
|
Hi @alfonsogarciacaro, sorry for the delay 🙏 I have now merged and published this implementation as of Feliz.UseElmish v1.6 🚀 |
This fixes fable-compiler/fable-promise#24 following @MangelMaxime suggestion to use the Elmish.Program implementation instead of adding a custom one.
I've tested this in a project of mine and also in Fable.Lit and it seems to work. Feliz tests are also passing. The PR is still missing the dependencies, but if you're open to merge this @Zaid-Ajaj I can add them too.