Set root component state instead of calling RenderDom for every update

[alfonsogarciacaro]

Alternative to #6

[et1975]

Would you mind running performance test, with something like this.

[alfonsogarciacaro]

I cloned the repo but there are no instructions to build the tests. Do you know how to do it?

[MangelMaxime]

@alfonsogarciacaro I had already added Elmish test some months ago in my fork. Please take a look at the 2 last commits.

You will probably need to update the Fable version, etc. because it's outdated but you will got the basic idea of how to add a new test.

[alfonsogarciacaro]

Thanks @MangelMaxime! It seems I couldn't run the tests because picker.js was missing. I don't have Elm to compile it so I just copied it from the live example and added it to the repo. Besides that, I've updated the elmish-react sample and added a version with the editions in this PR. However, the test runner fails in this line because the DOM elements are not mounted synchronously so the buttons with classname "toggle" cannot be found. Any idea how can this be fixed? (I also tried removing requestAnimationFrame but that didn't work)

[zaaack]

@alfonsogarciacaro Have you tried with input's Value ? After adding requestAnimationFrame input's Value doesn't work in my here.

Looks in newest React it's already using requestAnimationFrame under the hood, maybe it's OK to remove it?
https://github.com/facebook/react/blob/master/packages/shared/ReactDOMFrameScheduling.js#L206

[alfonsogarciacaro]

@zaaack Yes, input's Value is working. You can try my cloning my fork and do the following:

cd implementations/elmish-react-setstate
yarn
dotnet restore
dotnet fable yarn-start

About the link to the React code, I didn't follow the references but in the comment on the top of the file it says that's a polyfill for requestIdlCallback, which according to MDN:

queues a function to be called during a browser's idle periods. This enables developers to perform background and low priority work on the main event loop, without impacting latency-critical events such as animation and input response.

That makes me wonder whether this is actually used for ReactDom.render or setState, as these are hardly considered low priority work 🤔

[zaaack]

@alfonsogarciacaro The input's Value doesn't work in my here after I change your raf code to:

match rootEl with
| Some _ ->
        // observable.Trigger(model, dispatch)
        match lastRequest with
        | Some r -> window.cancelAnimationFrame r
        | None -> ()
                lastRequest <- Some (window.requestAnimationFrame (fun _ ->
                    observable.Trigger(model, dispatch)))

I didn't look deeply into React's source code, and cannot give some strong evidence that React using requestIdlCallback to batch setState. But I think it's not a big problem since we can easily add a new ui test case to find out the performance difference.

[alfonsogarciacaro]

@zaaack Ah, you're right. The cursor is jumping to the end if requestAnimationFrame is used, didn't notice that 😅 Well, then I guess if the whole purpose of using setState is to avoid that glitch we'd have to remove raf too 😸

[zaaack]

local state might break hmr, https://github.com/fable-elmish/react/pull/6/files#diff-6e208d7e773cd65ba4c7e6cf6adde006R36 maybe it's better to be put in module level?

[alfonsogarciacaro]

Makes sense, thanks! I was also thinking that a boolean flag (like let mutable initialized = false) is probably enough here 👍

[zaaack]

I tried move the local state to global, remove shouldComponentUpdate, but it's strange that I cannot get HMR working with you implementation. you can try it with my demo repo by replace paket-files/zaaack/react/src/react.fs with yours and edit Home/View: https://github.com/zaaack/elmish-react-demo

[alfonsogarciacaro]

Hi @zaaack! I just did that: cloned your demo and replaced react.fs with the code here (also uncommented the |> Program.withHMR line in App.fs) and HMR worked without problem. The only change needed is to remove shouldComponentUpdate so I guess we can wrap it #if !DEBUG to keep it in Release mode. Note I didn't have to make any state global.

[zaaack]

@alfonsogarciacaro I tried what you said before, but the view doesn't change if I edit a string child of a ReactElement, and also no errors.

[MangelMaxime]

Don't know ..

[zaaack]

I would vote for not using requestAnimationFrame...

[zaaack]

@alfonsogarciacaro I tried my react.fs and hmr works for update. And also yours after making local state to global state like this:

(Sorry for the downcast, I don't know how to get around Value restriction and just for a quick example, so ... 😅)

open Fable.Import.Browser

    let mutable observable = None :> obj

    /// Setup rendering of root React component inside html element identified by placeholderId
    let withReact placeholderId (program:Elmish.Program<_,_,_,_>) =

        let setState model dispatch =
            match (observable :?> SingleObservable<_> option) with
            | Some obs ->
                obs.Trigger(model, dispatch)

In my case local state are lost after hmr, but in your code it's initialized again, it can be easily find out by adding some logs. But still don't know why your logical code are not update?

[alfonsogarciacaro]

I think I'm getting a bit lost ;) I don't see any difference by placing observable on the top level. But yes, I can confirm requestAnimationFrame is what causes the jumping cursor issue, if I add it to these lines then the problem appears again.

[zaaack]

After I making local state to global, the new-added printfn("update") in update function worked, before it doesn't print anything.

[et1975]

So, how is the perf looking?

[et1975]

Actually, forget performance. Can you put the old impl back in, just name it something like withReactDOM, for people like me who don't want the full React just for that little glitch?

[alfonsogarciacaro]

That's a great idea @et1975! I assume keeping the old implementation means also keeping requestAnimationFrame (which seems to be the actual cause of the glitch), is that right?

About performance, I haven't managed yet to make this work with the test project (see above), I'll see if it can be fixed 👍

[MangelMaxime]

@alfonsogarciacaro I think he means to not touch the withReact function and create a new one called withReactDom which is the one you propose in this PR.

Like so users have the choice.

[et1975]

Either way, just as long as there's a choice.

[alfonsogarciacaro]

Ok, I finally found why the tests didn't work, the runner is submitting input events so this event must be changed to OnInput. After doing that the tests have revealed... this PR is actually slower 😅

However, removing requestAnimationFrame gives better results and removes the glitch (as @zaaack has been saying all the time 😉), should we do just that?

[zaaack]

Thanks for your effort to fix this issue! I can confirm requestAnimationFrame is the reason. If removing requestAnimationFrame can get better performance, then I suppose React's optimization is undering ReactDOM.render, and removing it is good for everyone!

Maybe there are some info that I forgot to mention, this "glitch" would cause asians (like me) who write CJK with IME cannot input any character, we have to using the workaround anywhere. Perhaps that's also why there are not much feedback about this problem...

But still, thanks a lot!

[alfonsogarciacaro]

Thanks to you for such a thorough testing @zaaack I also use CJK with IME (though I hadn't tried with an Elmish app yet) so I agree this is a strong reason to fix the glitch (I also fixed something similar in Fable). What do you think @et1975? If that's ok, I will edit the PR so it just removes requestAnimationFrame and nothing else (well, probably also caching the location of the DOM element when React is mounted), or @zaaack can also do it in his PR 👍

[et1975]

For backwards compatibility, let's have 2 functions to init react: withReactComponent where you can have your component instantiated and run any way you like, and withReact that retains my earlier implementation.

[alfonsogarciacaro]

Sorry, I should have just updated the PR to make the intention clearer. I just did it and as you can see, the (last) proposal is to only remove requestAnimationFrame, no special React component is needed. Just by removing requestAnimationFrame the glitch is fixed.

I run the tests again, and this PR is also slower, but I guess requestAnimationFrame has an unfair advantage for this kind of benchmarks which will probably fade away in a normal application.

What do you think?

[et1975]

Hmm, weird! So this must have been a regression when I introduced the RAF optimization...
I wonder how exactly it screws React, because even though it seems like it would be harmless to apply this patch I'd really like to keep the RAF. Here's my reasoning: while a human can't type fast enough for this optimization to make a difference, something like websockets flooding with updates can easily cause so many redraws that UI appears to freeze. I wish we could pull a react dev into this to explain what's happening... Anyone got friends at Facebook? :)
Failing that... let's think this through... RAF should make no difference when a human is typing, right? And yet it does? So what's different?

[MangelMaxime]

I am a bit loss here. In the latest state of this PR what is it trying to solve ?

Because, at first it was about using setState ? But now, it's about removing the RAF ?

Aparté
Also, in the test benchmark Elm is not using RAF. Source so the benchmark should not use it either.

[alfonsogarciacaro]

@MangelMaxime Yes, this and #6 where originally intended to use setState in the root, because we thought this was causing the glitch. But with the tests and thanks to @zaaack's help, we realize the real issue was the requestAnimationFrame optimization, that's why I changed the PR.

Yes, Elms is not using requestAnimationFrame in the tests so it doesn't have an unfair advantage, and we do, but that's another topic 😉

@et1975 Following your previous suggestion, I've modified the PR to keep the original implementation and offer an alternative with the explanation. I also opened #12 to keep better track of the issue and try to investigate the real cause of the problem 👍

[et1975]

I've released this as 1.0.1-beta-1, please hard reset on origin/master as I've squashed all those commits to keep the history clean.