Using setState to render the view

[zaaack]

Fix elmish/elmish#60

This issue has been fixed in react long long ago, but it appears in elmish-react now. If we using setState to update the input's value, it won't reset the cursor, because the real input's value after typing is the same as react vdom's, so react won't change the real dom, and cursor won't change. But if we update the view by ReactDOM.render, this issue occurred again, so I think we could change to the commonly used way (update the view by setState) in react to keep the consistency.

Warning: this is a breaking change, in the original way, DefaultValue can work around it just like Value in this new way, if you change DefaultValue in model, it would update the view, too. But now, DefaultValue is the default value, and won't sync to view if the view is not first render. If you need to control the input, you should using Value instead, which is the same as in normal react app.

[MangelMaxime]

@et1975 For info,
this is breaking HMR support for Elmish.

And I can't use it on a simple input, it's failing at runtime with f is not a function when I try to dispatch a message into my application. I tried to dispatch a message from a button same problem.

[zaaack]

@MangelMaxime Can you try it again? Looks I ran into some fable issue maybe? it works now in my elmish-react-template app. Didn't notice HMR is breaking, I'll see what I can do.

[zaaack]

Looks like the previous code triggered a wired webpack/babel fable bug, it translate a function to an IIFE

code from repl:

export const hasUpdate = defaultArg(defaultArg(lastModel, null, function (a) {
  return !(model === a);
} /*a function */), true);

code from webpack:

 var hasUpdate = Object(__WEBPACK_IMPORTED_MODULE_8__nuget_packages_fable_core_1_3_0_fable_core_Option__["b" /* defaultArg */])(Object(__WEBPACK_IMPORTED_MODULE_8__nuget_packages_fable_core_1_3_0_fable_core_Option__["b" /* defaultArg */])(lastModel, null, function (a) {
                    return !(model === a);
                }())/* an IIFE */, true);

Still trying to understand how HMR works.

===== Update ====

This bug comes from fable, I repoduced it with fable-splitter and dotnet-fable 1.3. Just created an issue in fable. @alfonsogarciacaro

[zaaack]

@MangelMaxime Now HMR is working, again! Thanks a lot for your test!

[MangelMaxime]

Please remove this debug log :)

[MangelMaxime]

HMR seems to works, and I don't see performance problem from my tests.

Will let Eugene take the relay from now.

[zaaack]

@MangelMaxime Updated!

[et1975]

This issue has been fixed in react long long ago

Clearly some optimization is taking place, but apparently in React impl it happens at model->VDOM stage, instead of VDOM->DOM (as Preact appears to be doing it). If that's the optimization you want why not implement that specifically - a function that takes some state (string) and returns Prop or Prop list suitable given the difference?

The only reason Native impl uses an app component is because Native doesn't work without. It's ugly and it was a massive pain to get it to work properly. Looking at your code I have no confidence that it will work correctly and you are not inspiring any by letting others find the issues instead of doing thorough testing yourself. Specifically, I find the use of rendered view as state troublesome conceptually, I also have a feeling that it would negate the benefits of lazy views (com<> will always return a new reference). What is the ultimate cost of this? I don't know, it's possible that certain elements (like the ones using D3, which are known to depend on lazy will be severely affected).

In general, not that I don't appreciate the effort, but the issue you are fixing is closed and the discussion around it clearly amounts to "there are known workarounds" not "send the PR!".

My suggestion is to keep this as a separate implementation of React renderer, in your own library.

[zaaack]

et1975 Thanks for your patient explanation. What do you mean a function that takes some state (string) and returns Prop or Prop list suitable given the difference? ? I don't think there is a way to change Value's default behavior without modify the library self.

I know maybe this looks a little hacky, but it totally followed the React's core API 😉. And In my imagine it should works like a drop-in replacement for original implementation, the root component (App) only rendered by ReactDOM.render once, so I don't need to wrap it with lazy view, I only wrapped it's children with lazyView. React don't do model diff in default, so there should be no cost in putting a big view into state (maybe different in RN? but I think I can wrap it with a function if it's better), since it will just update the view after calling setState.

This also won't break React's shouldComponentUpdate API, it still composes the components like a normal react app, so I think lazyView will still work, and I can confirm it:

In general, not that I don't appreciate the effort, but the issue you are fixing is closed and the discussion around it clearly amounts to "there are known workarounds" not "send the PR!".

To be clear, there are two main concerns of this PR:

1. People will always run into this issue, especially those comes from React. Then they need to use the DefaultValue workaround. DefaultValue isn't value but work just the same as value in normal React, and there even isn't an equivalent to defaultValue in React, maybe it's a bit confusing? What's more, what if people are using some third-party UI libraries that didn't implement DefaultValue at all?

2. Actually it's my first time that knows ReactDOM.render would using React's vdom diff, I think most react app only use it as mount reactApp to some dom node, so there is still a risk that there were or will be (with React's upgrade) other differences between ReactDOM.render and setState, and at that time it still need to change back to keep consistency and benefit from react's upgrade. And this is what I worried most.

But I totally agree that this need more feedback and testing, especially elmish-react have a big ecosystem and this is also a breaking change. Maybe release in alpha or beta channel? I'll definitely try and test it with my current fable project.

Update: view has already been wrapped by a render function when passing to state.

[alfonsogarciacaro]

Maybe you could push a package with a different name and let users test it to give you feedback, @zaaack? I'm a bit concerned that in this PR requestAnimationFrame is removed. I would rather keep it as it's a good way to increase performance when there are too frequent updates. I've also had sometimes the situation where I wanted to set the state of a component from the outside. My solution in those cases was to pass an observable and let the component subscribe to it. That may feel less hacky for this purpose, so I've sent PR #10 just for reference to how this could be implemented, but I haven't tested it yet.

[zaaack]

@alfonsogarciacaro Publishing a fork version is a choice, but I don't have confidence I can get enough feedback from a small community for a fork repo. Currently DefaultValue work around is still not a big problem, this pr might more like a prevention of potential issues/risks and user experience enhancement.

I thought React's setState is already well optimized and batched, so I removed the requestAnimationFrame code. But after some digging, looks adding RAF still can optimize react's performance, and react team are also considering adding it to react core.

Update: In newest React, it's already using RAF under the hood. https://github.com/facebook/react/blob/master/packages/shared/ReactDOMFrameScheduling.js#L138

Your observable approach is much clear 👍, either of them getting merged is OK for me. Even if neither of these can, I think I can still reference a fork repo by paket, which others can also help testing.

[et1975]

For completeness, this is the function that I think replicates the React optimization in its entirety:

    let inline valueOrDefault value e = 
        if e |> isNull |> not && !!e?value <> !!value then e?value <- !!value

Usage:

   R.input [Ref (valueOrDefault model.Value); OnChange ... ]

[cgravill]

Just to add to my previous thumbs up, I'm using your code @et1975 and it works in all use cases I've tried so that's great thanks.