Skip to content

fixes root mounting and optimizes view functions#114

Merged
et1975 merged 1 commit intoelmish:v5.xfrom
et1975:v5.x
Mar 3, 2026
Merged

fixes root mounting and optimizes view functions#114
et1975 merged 1 commit intoelmish:v5.xfrom
et1975:v5.x

Conversation

@et1975
Copy link
Member

@et1975 et1975 commented Mar 1, 2026

Fix root remounting and modernize lazy views with React.memo

Summary

Replaces the class component-based LazyView implementation with React's memo API, fixes root element remounting on each render loop iteration, and introduces a withKey helper for element reconciliation. Closes #14 (however suboptimally).


Changes

Core: Replace LazyView class component with React.memo (src/common.fs)

  • Removed LazyProps, Components.LazyView class component, and Internal.ofType helper
  • lazyViewWith, lazyView2With, and lazyView3With now create memoized functional components via React.memo with a JS-level __memo stamp on the view function for stable component identity
  • Memoized components automatically inherit the F# view function's name as displayName for better React DevTools experience
  • Breaking (minor): lazyView2With and lazyView3With now return curried functions — designed for partial application (let render = lazyView2With equal view) so the memo component is created once and reused
  • Added MemoProps1 / MemoProps2 record types to carry model/dispatch/equal through memo props
  • Added withKey function to set React keys on elements, enabling sibling differentiation for memo components (See if we can move key prop from a view we're wrapping into the element produced by lazyView #14)

Program: Fix root remounting (src/react.fs)

  • withReactBatchedUsing, withReactSynchronousUsing, and withReactHydrateUsing now partially apply lazyView2With once at setup time rather than on every setState call
  • Prevents creating a new memo component type on each render cycle, which caused React to unmount/remount the root element

Tests: Comprehensive test suite

  • tests/LazyViewTests.fs — 351 lines covering:
    • Memo skip/re-render behavior for all lazyView variants
    • Component identity (same partial application = same type)
    • Inline usage (no partial application) via __memo stamping
    • displayName propagation from named F# functions
    • withKey correctness, type preservation, and keyed reorder without remount
  • tests/Helpers.fs — Test utilities (flushSync, getDisplayName, getElementKey, etc.)
  • tests/DisplayNameViews.fs — Named view functions for displayName assertions
  • tests/Main.fs / tests/Tests.fsproj — Test runner setup with Fable + Mocha + jsdom

Infrastructure

  • Upgraded react / react-dom from ^17.0.2 to ^18.0.0
  • Added mocha, jsdom, global-jsdom dev dependencies
  • Added npm test script
  • Added .config/dotnet-tools.json for local tool manifest

React DevTools Components view post-optimization

image

@et1975 et1975 requested a review from MangelMaxime March 1, 2026 04:18
"react": "^17.0.2",
"react-dom": "^17.0.2",
"react": "^18.0.0",
"react-dom": "^18.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not using 19?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, conversely there doesn't appear to be such a drastic difference between 18 and 19 as between 17 and 18 (fibers) wrt to changes I'm making here. As this is a dev-only dependency, it shouldn't matter to consumers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to release react 19 support with a major version bump since it ditches root.render(). There also seem to be no need for memo...

Copy link
Contributor

@kerams kerams Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React 19 support has already been added;) root.render is the new API added in 18.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, you're right... I remember you did that a year ago. I wonder why we didn't update devDependencies back then. Maybe @MangelMaxime needs it for that version of nacara?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nacara do use React indeed, but I should be able to update its React version to the latest.

I am mostly using it to render HTML so I don't really use any fancy React API.

@et1975 et1975 requested a review from kspeakman March 1, 2026 16:14
@et1975
Copy link
Member Author

et1975 commented Mar 2, 2026

Released as 5.5.0-beta-1, would be great if people gave it a spin and reported how they like it.

@kerams
Copy link
Contributor

kerams commented Mar 2, 2026

.\src\Client\fable_modules\Fable.Elmish.HMR.7.0.0\common.fs(65,9): (65,15) error FSHARP: The value or constructor 'ofType' is not defined. (code 39)

Will need a bump there too.

@kspeakman
Copy link

I'll try to give this a look tonight.

@et1975
Copy link
Member Author

et1975 commented Mar 2, 2026

@MangelMaxime I created HMR PR as well elmish/hmr#48

@MangelMaxime
Copy link
Member

Program: Fix root remounting (src/react.fs)

  • withReactBatchedUsing, withReactSynchronousUsing, and withReactHydrateUsing now partially apply lazyView2With once at setup time rather than on every setState call

  • Prevents creating a new memo component type on each render cycle, which caused React to unmount/remount the root element

As we are trying to fix root remounting, should we try to fix it for HMR too?

When applying HMR we are getting:

Warning: You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it. react-dom.development.js:86:1

I believe this is because every time HMR trigger are re-running the withReactXXX APIs which calls :

let root = ReactDomClient.createRoot (document.getElementById placeholderId)

A solution to that problem would be to attach the root, to the mounted HTML element:

// Pseudo code (can probably be done with `match ... with`
let rootElement = document.getElementById placeholderId
let root = 
	if isNull (rootElement?__elmish_root) then
    	let root = ReactDomClient.createRoot root
		rootElement <- root
		root
	else
		rootElement?__elmish_root

In theory, this should allow React to correctly unmount the previous React components.

I don't know if this could have unwanted side effects or not, it would avoid having a warning in dev mode, confusing some new users.

@et1975
Copy link
Member Author

et1975 commented Mar 3, 2026

@kerams Maxime has published the HMR beta, could you give it a spin?

@et1975
Copy link
Member Author

et1975 commented Mar 3, 2026

Guess I'll merge this and we can do another round of changes as needed for GA.

@et1975 et1975 merged commit 09bb1f6 into elmish:v5.x Mar 3, 2026
2 checks passed
@kerams
Copy link
Contributor

kerams commented Mar 4, 2026

So far so good. Anything I should watch out for?

@et1975
Copy link
Member Author

et1975 commented Mar 4, 2026

@kerams not in particular, thanks for pointing out the HMR issue.

@kspeakman
Copy link

Sorry I didn't get to this sooner. You move fast. Looks good to me. Testing will tell the tale.

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.

See if we can move key prop from a view we're wrapping into the element produced by lazyView

4 participants