UseElmish with useSyncExternalStore

[alfonsogarciacaro]

Fix #481

This PR updates UseElmish to use useSyncExternalStore, which seems to be the recommended hook for this use case in React 18. The code is greatly simplified, we don't need the ElmishObservable or ResumableState anymore.

It took a bit of trial-and-error to use the hook correctly but I've tested it and it's working fine with hot reloading.

The PR also upgrades the code to Elmish 4.

~~One breaking change is that I removed the dependencies argument. This is because the code now only uses two React hooks: useState and useSyncExternalStore, and neither has a dependencies argument. To support dependencies we would have to add useEffect or our own logic complicating things. It's already tricky to get the life cycle right and Elmish 4 also introduces termination capabilities, so I'd prefer not to add extra logic to restart the state. If you need to reset the model if a dependency changes, we can create a specific message for this and handling it in the update function.~~

@MangelMaxime @Dzoukr

[MangelMaxime]

Thank you @alfonsogarciacaro for looking into this.

I didn't review the code yet, but have a question.

If you need to reset the model if a dependency changes, we can create a specific message for this and handling it in the update function.

I don't understand how this will work.

In Fable.Packages, we have one Elmish components which has a dependency over props.UrlParameters making it possible to adapt to changes of the URLs.

    [<ReactComponent>]
    static member SearchForm(props: SearchFormProps) =
        let model, dispatch =
            React.useElmish (
                init props.UrlParameters props.OnSearch,
                update props.OnSearch,
                [|
                    box props.UrlParameters
                |]
            )

        Html.div [
            Components.SearchInputField model dispatch
            Components.Filters(model, dispatch)
        ]

https://github.com/fable-compiler/packages/blob/232b5ec1322a30e16ce2bcddc37cff9f007fc201/src/Components/SearchForm.fs#L462-L476

How would this work in this case?

[alfonsogarciacaro]

Sorry, scratch the part about the dependencies, I had a wrong understanding about how the Elmish program was being terminated 😅 I added the dependencies back and also add a check for arg because I assume if the init argument changes, users would expect the component to refresh.

@Zaid-Ajaj I've updated react in package.json to v18 to make the tests work but now the "lazy and suspense works" test doesn't work, not sure why. For now I just commented it out, is that OK? I've also noticed that when running webpack in production mode tests fail too, but I haven't investigated it either.

Also, I've removed the Feliz dependency from Feliz.UseElmish (I left only Feliz.CompilerPlugins for the Hook plugin). In my tests, I didn't notice a problem with the Feliz.UseElmish.React type shadowing the Feliz.React type but please let me know if you find something. I removed the dependency so Fable.React users can download UseElmish without bringing the full Feliz dependency. And we also allow Feliz users to update UseElmish without having to update Feliz to v2 (to avoid breaking changes).

[alfonsogarciacaro]

Sorry @Zaid-Ajaj, I did a force push and this seems to confuse Github notifications. This is ready to merge when you have a moment to look at (but please check comment above).

BTW, would it be possible to make @MangelMaxime and I also owners of the Feliz.UseElmish Nuget package so we can make new releases if necessary?

[alfonsogarciacaro]

Actually, I just realized if I decorate the type with CompiledName("useReact") I can get the same effect as when using the Hook attribute. Anyways, in my tests there didn't seem to be a difference between prefixing with use- or not, maybe that has been solved already?

Also, when using JSX.Component it may happen there is no import React in the file at the end. But I got hot reload working anyways. @MangelMaxime where did you see that import React is necessary? I've only seen it for the case of compiling JSX, but it seems this is not necessary with the new JSX runtime.

This is the summary of the things I've noticed when using @vitejs/plugin-react for Fast Refresh:

• Having an import React clause doesn't seem to be necessary
• Using export default doesn't seem to be necessary
• However, there must be only one export for Fast Refresh to work. There can be other components in the file (and they will trigger fast refresh too) but they must be private.

About the 3rd point, one must be careful when using a static class in order to have optional arguments. In that case the class must be decorated with Erase so the code is not output adding an extra export.

Also, given that UseElmish doesn't have a Feliz dependency anymore, would it make sense to host it in its own repo and have more maintainers for the package? Should we change the namespace in that case (e.g. Fable.React.UseElmish)?

[alfonsogarciacaro]

@Zaid-Ajaj npm test is failing, but this is still running with Fable 2. npm run test:nagareyama works for me locally. Do we still want to maintain the tests for Fable 2 or can we skip them? (BTW, I'm also surprised the tests for Fable 3 were working even after updating the plugin)

[Zaid-Ajaj]

Hi @alfonsogarciacaro sorry for the late reply here. I will take a closer look at the PR as soon as possible 🤞

[alfonsogarciacaro]

Thanks @Zaid-Ajaj! I made a change in this commit that fixes the Fable 2 tests, but it exposes a useElmish function besides the React overloads. I don't have a strong opinion on this, it looks like prefixing with use if not necessary now, but if you want we can add back the Hook plugin.

[MangelMaxime]

@MangelMaxime where did you see that import React is necessary?

I it based on reading the code of react-refresh, and also by experimenting.

I did the experimentation by manually editing the JavaScript, F# files, etc.

It is important to note, that if you are using JSX syntax then Vite, Babel, and other plugin are mostly likely to add code for you.

For example, vite/plugin-react is going to prepend the generated code with import React from 'react';

https://github.com/vitejs/vite/blob/e976b065628ca451cd07bf5472427462d64bc56c/packages/plugin-react/src/index.ts#L98

https://github.com/vitejs/vite/blob/e976b065628ca451cd07bf5472427462d64bc56c/packages/plugin-react/src/index.ts#L283-L293

So, I fell like you think you are not adding import React from 'react'; in your F# code , but because you are using JSX it will still be present in the output.

However, there must be only one export for Fast Refresh to work. There can be other components in the file (and they will trigger fast refresh too) but they must be private.

I have to disagree, with this one. You can have multiple components exported in the same file and Fast refresh is still going to work. Fable.Packages does it for example.

module Fable.Packages.Components.AnErrorOccured

open Fable.Core.JsInterop
open Feliz
open Feliz.Bulma
open Fable.Packages

// Workaround to have React-refresh working
// I need to open an issue on react-refresh to see if they can improve the detection
emitJsStatement () "import React from \"react\""

type Components with

    [<ReactComponent>]
    static member AnErrorOccured
        (
            title: string,
            subtitle: string,
            details: string
        ) =
        // ...

    [<ReactComponent>]
    static member AnErrorOccured(title: string, subtitle: string) =
        // ...

I also made a sandbox, using pure JavaScript:

https://user-images.githubusercontent.com/4760796/197342224-5726635a-34a5-4fe2-ba70-5c2686f9a0fa.mov

[MangelMaxime]

@alfonsogarciacaro

Do you think it would be possible to use use-sync-external-store NPM package?

This package allow to have use-sync-external-store working for any version of react.

The drawback is that users will need to install react, react-dom and use-sync-external-store NPM packages. But this is kind of the recommanded way of doing it.

[Zaid-Ajaj]

However, there must be only one export for Fast Refresh to work. There can be other components in the file (and they will trigger fast refresh too) but they must be private.

Last time I checked, there could be multiple exports for React components in one file, so this should be no problem AFAIK

Do you think it would be possible to use use-sync-external-store NPM package?

It would be great to do this @alfonsogarciacaro! People who use Feliz < 2.0.0 can still use the updated Feliz.UseElmish without having to upgrade everything at once 🙏 could you please have a look at that?

[alfonsogarciacaro]

Using use-sync-external-store makes sense, we had a similar discussion here: https://github.com/alfonsogarciacaro/fable-react-sample/issues/1#issuecomment-1286542980

In any case, please note that:

• this avoids having to update to React 18 (which is important)
• has no influence on Feliz/Fable.React versions (as discussed above, if we remove the Hook attribute we don't even need Feliz nor Fable.React as dependencies).
• still requires updating to Elmish 4 (which may have an impact on devs using "global" Elmish and UseElmish at the same time)

My only concern is that I have already released Fable.React.UseElmish and I'm reluctant to push a new version with a new npm dependency. But maybe we could publish Feliz.UseElmish with use-sync-external-store dependency so users who don't want to update to React 18 yet can use it.

As an aside, the React import may has to do with the new JSX transform in React 17, the linked web says this avoids the React import requirement. And as an aside x2, this makes me wonder if we should use jsx in the ReactComponent plugin instead of React.createElement as the former brings some performance improvements.

[MangelMaxime]

@alfonsogarciacaro I am not sure what Fable.React.UseElmish is?

Is it a fork/port of the code from you PR ? Or is it something else?

Using the new JSX transform make sense I guess.

My only concern is that I have already released Fable.React.UseElmish and I'm reluctant to push a new version with a new npm dependency.

Make a major bump as it is a breaking changes.

If the package is missing, the bundler will complain about it and user will be inform.

You could also, in DEBUG add a check + custom exception explaining the situation but I think the bundler should be enough.