-
Notifications
You must be signed in to change notification settings - Fork 91
Fix: Feliz.Elmish SSG #697
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the useElmish hook to support Static Site Generation (SSG) by refactoring how useSyncExternalStore interacts with F# functions. The key changes involve removing custom JavaScript interop and introducing delegate types for better SSG compatibility.
Key changes:
- Introduced
UseSyncExternalStoreSubscribeandUseSyncExternalStoreSnapshot<'T>delegate types for type-safe React interop - Refactored
React.useSyncExternalStoreoverloads to use delegates instead ofFunc<>types - Updated
UseElmish.fsto use the Feliz namespace and new delegate types, eliminating custom JS interop
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Feliz/ReactBindings/UseSyncExternalStore.test.fs |
Removed extra blank line (whitespace cleanup) |
src/Feliz/React/ReactTypes.fs |
Added delegate type definitions for useSyncExternalStore |
src/Feliz/React/React.fs |
Updated useSyncExternalStore overloads to use new delegate types |
src/Feliz/React/FsReact.fs |
Added helper function to create UseSyncExternalStoreSubscribe delegate |
src/Feliz.UseElmish/UseElmish.fs |
Removed custom JS interop, now uses Feliz API with delegates |
src/Feliz.UseElmish/Feliz.UseElmish.fsproj |
Added project reference to Feliz.fsproj |
docs/docs/ecosystem/04_Hooks/Feliz.UseElmish.mdx |
Updated to show live example with BrowserOnly wrapper for SSG support |
Comments suppressed due to low confidence (1)
src/Feliz/React/React.fs:664
- In the overload for useSyncExternalStore that accepts IDisposable, the getSnapshot parameter should also be converted to the delegate type. Currently, it's expecting UseSyncExternalStoreSnapshot<'T> but when calling with a function, the caller would need to wrap it in the delegate. Consider adding an overload that accepts (unit -> 'T) for getSnapshot and converts it internally, similar to how subscribe is handled.
static member inline useSyncExternalStore(subscribe: (unit -> unit) -> #IDisposable, getSnapshot: UseSyncExternalStoreSnapshot<'T>, ?getServerSnapshot: UseSyncExternalStoreSnapshot<'T>): 'T =
React.useSyncExternalStore( UseSyncExternalStoreSubscribe
(fun (callback) ->
let disp = subscribe(callback)
fun () -> disp.Dispose()
),
getSnapshot,
?getServerSnapshot = getServerSnapshot
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <CodeBlock language="fsharp" showLineNumbers> | ||
| {RawElmishCounter} | ||
| </CodeBlock> | ||
| <ComponentRender code={RawElmishCounter}><ElmishCounter /></ComponentRender> |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation shows duplicate ComponentRender elements rendering the same component. Line 37 should be removed as it's redundant with the BrowserOnly wrapped version below it. The BrowserOnly wrapper is the correct approach for components that don't support SSR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only single one now
| static member inline useSyncExternalStore(subscribe: (unit -> unit) -> (unit -> unit), getSnapshot: UseSyncExternalStoreSnapshot<'T>, ?getServerSnapshot: UseSyncExternalStoreSnapshot<'T>): 'T = | ||
| React.useSyncExternalStore( UseSyncExternalStoreSubscribe subscribe, getSnapshot, ?getServerSnapshot = getServerSnapshot) |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the overload for useSyncExternalStore that accepts an F# function signature, the getSnapshot parameter should also be converted to the delegate type. Currently, it's expecting UseSyncExternalStoreSnapshot<'T> but when calling with a function, the caller would need to wrap it in the delegate. Consider adding an overload that accepts (unit -> 'T) for getSnapshot and converts it internally, similar to how subscribe is handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works with the optional parameter. Not really sure how it works with the question mark on the sender side.
src/Feliz/React/FsReact.fs
Outdated
| token | ||
|
|
||
| static member inline createSyncExternalStoreSubscribe(subscribe: (unit -> unit) -> (unit -> unit)) : UseSyncExternalStoreSubscribe = | ||
| UseSyncExternalStoreSubscribe(subscribe) |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a helper function to create UseSyncExternalStoreSnapshot delegate, similar to createSyncExternalStoreSubscribe, to provide a consistent API for users who need to create these delegates manually.
| UseSyncExternalStoreSubscribe(subscribe) | |
| UseSyncExternalStoreSubscribe(subscribe) | |
| static member inline createSyncExternalStoreSnapshot<'Snapshot>(getSnapshot: unit -> 'Snapshot) : UseSyncExternalStoreSnapshot<'Snapshot> = | |
| UseSyncExternalStoreSnapshot(getSnapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the helper as it seemed unnecessary and more verbose
| if state.IsOutdated(arg, dependencies) then | ||
| ElmishState(program, arg, dependencies) |> setState | ||
| let finalState, dispatch, subscribed, queuedMessages = useSyncExternalStore(state.Subscribe, (fun () -> state.State), None) | ||
| let finalState, dispatch, subscribed, queuedMessages = React.useSyncExternalStore(state.Subscribe, UseSyncExternalStoreSnapshot(fun () -> state.State), UseSyncExternalStoreSnapshot(fun () -> state.State)) |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same getServerSnapshot value (state.State) is being used for both the client and server. This defeats the purpose of SSG support. For SSG to work correctly, getServerSnapshot should typically return a stable initial state or handle the case differently from the client-side snapshot. Using the same value for both parameters means SSG/SSR will still encounter the same issues this PR is meant to fix.
| let finalState, dispatch, subscribed, queuedMessages = React.useSyncExternalStore(state.Subscribe, UseSyncExternalStoreSnapshot(fun () -> state.State), UseSyncExternalStoreSnapshot(fun () -> state.State)) | |
| let initialState = state.State | |
| let finalState, dispatch, subscribed, queuedMessages = | |
| React.useSyncExternalStore( | |
| state.Subscribe, | |
| UseSyncExternalStoreSnapshot(fun () -> state.State), | |
| UseSyncExternalStoreSnapshot(fun () -> initialState) | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my tests and understanding this should be fine as the initial value should always be the same that is passed in from the init function.
|
|
||
| import ComponentRender from '@site/src/components/ComponentRender'; | ||
| import CodeBlock from '@theme/CodeBlock'; | ||
| import BrowserOnly from '@docusaurus/BrowserOnly'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you correctly handle the getServerSnapshot variable it should work without BrowserOnly component https://react.dev/reference/react/useSyncExternalStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional getServerSnapshot: A function that returns the initial snapshot of the data in the store. It will be used only during server rendering and during hydration of server-rendered content on the client. The server snapshot must be the same between the client and the server, and is usually serialized and passed from the server to the client. If you omit this argument, rendering the component on the server will throw an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was from an earlier test to just show the docs before I attempted making useElmish for SSG. Updated docs to remove duplicate and import.
…a real running example
…scribe instead of helper
This fixes issue with the useElmish hook not working with SSG.
I removed the custom JS interop (maybe not the best idea) to keep it consistent which also affected how the base functions for useSyncExternalStore worked.
Tests and Docs (in build) were building for me along with another test project.
Feedback welcome, especially on the delegates.