Skip to content
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

Fast Refresh #157

Closed
Shmew opened this issue Apr 10, 2020 · 26 comments
Closed

Fast Refresh #157

Shmew opened this issue Apr 10, 2020 · 26 comments

Comments

@Shmew
Copy link
Collaborator

Shmew commented Apr 10, 2020

The React team is currently in the process of implementing a native HMR solution which deprecates things like react-hot-loader called Fast Refresh.

I figured we should add an issue to keep this on the radar.

There are a some benefits to be gained from this:

  • Feliz apps won't need to use two different styles in the entry point of the application.
  • Preserves component state.
  • Doesn't destroy the DOM.
  • Not dependent on webpack
  • Works with React-Native.
  • Faster refreshing (if they live up to their name 😄)

In current state there are some logistics that need to be figured out:

  • Code block that must happen before anything else that injects some window variables.
  • All modules that export react components need to be wrapped in a try .. finally block and some other things to do with Fast Refresh.
@MangelMaxime
Copy link
Contributor

MangelMaxime commented Apr 10, 2020

IHMO if possible we should add support for it in Elmish.HMR because that's the current library responsible for handling HMR.

Feliz doesn't know about the application state when used with Elmish.

It's also important to note that Elmish.HMR will allow us to support any Elmish application like pure Fable.React, pure Feliz, a mix of both or even just pure Elmish with any renderer.

@Shmew
Copy link
Collaborator Author

Shmew commented Apr 10, 2020

Yeah that sounds fine to me, I wasn't sure if Elmish.Debugger or Elmish.HMR were necessarily the right places, I hadn't looked to see if they were directly coupled with React or not.

@MangelMaxime
Copy link
Contributor

Oops it's Elmish.HMR and not Elmish.Debugger I edited my previous message.

No, it's not coupled to React, Elmish.HMR shadows some of the Elmish features in order to make them support HMR.

For example:

  • Program.run from Fable.Elmish
  • Program.toNavigable from Fable.Elmish.Browser
  • Program.withReactBatched from Fable.Elmish.React
  • etc.

@Zaid-Ajaj
Copy link
Owner

Zaid-Ajaj commented Apr 10, 2020

@MangelMaxime AFAIK Elmish.HMR doesn't handle function components from React and the state is reset when you use them. I will try to make a repro soon

@MangelMaxime
Copy link
Contributor

MangelMaxime commented Apr 11, 2020

@Zaid-Ajaj From HMR point of view, that's totally normal.

I already have an issue tracking the situation. elmish/hmr#26

What you are describing is something well know in JavaScript and it's also one of the main problems of HMR. If you want to have HMR in your project you need to make your code understand it. This is not something that works out of the box.

In Elmish, most of the time you don't see it because the state is in a single place and Elmish.HMR learned to Elmish how to handle it. It's done by shadowing the functions.

In the case, of stateful component, you need to write the component in a way it can understand HMR. In order, to have it freely the best would be to implement it directly in the library providing the stateful component otherwise people will have to make sure to open the modules in the right order.

For example, if I shadow equalsButFunctions in Elmish.HMR then people need to remember to do:

open Fable.React
open Elmish.HMR

in order to have HMR supported.

But if the support for HMR is done directly in Fable.React code then they can just open Fable.React and that's it.

In general, you should not have impact on the bundle size because you can isolate the HMR code between #if DEBUG ... #else ... #endif

@albertwoo
Copy link

I find something which maybe useful.
Here is my repo to try.

After run dotnet fsi build.fsx, the output js will put under src\Client\www\fablejs
My Counter functional component is like:

[<ReactComponent>]
let Counter () =
    let (count, setCount) = React.useState(0)
    Html.div [
        prop.style [ style.padding 20 ]
        prop.children [
            Html.h1 count
            Html.button [
                prop.text "Increment"
                prop.onClick (fun _ -> setCount(count + 2))
            ]
        ]
    ]

It will generate js like below. Under folder src\Client\www\fablejs\Counter.js I can edit and trigger parcel to update.
I replaced old reactElement with createElement then save the Counter.js and the fast refresh starts to work. And it works together with elmish HMR (wow nice:)).
Alias (reactElement as createElement) will not work. Only import { createElement } from 'react'; seems work.
I also found some thing related to createElement in react-refresh: https://github.com/facebook/react/blob/ebf158965f2b437515af0bed2b9e9af280e0ba3c/packages/react-refresh/src/ReactFreshBabelPlugin.js#L192

import { Feliz_React__React_useState_Static_1505 } from "./www/fablejs/.fable/Feliz.1.16.2/React.fs.js";
import { reactApi, reactElement, mkAttr, mkStyle } from "./www/fablejs/.fable/Feliz.1.16.2/Interop.fs.js";
import { ofArray, singleton } from "./.fable/fable-library.3.0.0-nagareyama-rc-007/List.js";
import { createObj } from "./.fable/fable-library.3.0.0-nagareyama-rc-007/Util.js";
import { createElement } from 'react';

export function Counter() {
    let properties, elems, xs;
    const patternInput = Feliz_React__React_useState_Static_1505(0);
    const setCount = patternInput[1];
    const count = patternInput[0] | 0;
    const xs_1 = ofArray([(properties = singleton(mkStyle("padding", 20)), mkAttr("style", createObj(properties))), (elems = [createElement("h1", createObj(singleton(["children", new Int32Array([count])]))), (xs = ofArray([mkAttr("children", "Increment"), mkAttr("onClick", (_arg1) => {
        setCount(count + 10);
    })]), createElement("button", createObj(xs)))], mkAttr("children", reactApi.Children.toArray(Array.from(elems))))]);
    return createElement("div", createObj(xs_1));
}

Hope we can make Fast Refresh works!!!

@Zaid-Ajaj
Copy link
Owner

Hi there @albertwoo,

Can you please update Feliz to v1.17.0 and see if it works for you? Because fast-refresh is working as of that version and the latest set of dependencies of webpack and friends. Please see SAFE.React where you can see how it is working there as is without changing reactElement to createElement

I am also noticing you are using Parcel. What version of Parcel is that? latest 2.0-beta doesn't implement fast-refresh properly, the nightly version works. See this discussion on the parcel repo

@albertwoo
Copy link

But the SAFE.React is using @pmmmwh/react-refresh-webpack-plugin which I think is different with the fast refresh from react repo right? Maybe I am wrong.
But if I change reactElement to createElement it works correct. Isn`t it better to make the generated code close to react style as much possible?

I tried to upgrade Feliz to 1.17.0. And I was using parcel 2.0.0-nightly.447. But still not work by default.

@Zaid-Ajaj
Copy link
Owner

But the SAFE.React is using @pmmmwh/react-refresh-webpack-plugin which I think is different with the fast refresh from react repo right? Maybe I am wrong.

The webpack plugin is using fast-refresh as a peer dependency v0.9.0 can you please check inside your package-lock-json file which version of fast-refresh is being used and whether is it using latest?

Isn`t it better to make the generated code close to react style as much possible?

It is. However, reactElement is simply an alias for createElement so they should be exactly the same if the fast-refresh plugin is recursively going through the imports (which I think it is doing since it works in SAFE.React). I will see what I can do about this

I tried to upgrade Feliz to 1.17.0. And I was using parcel 2.0.0-nightly.447. But still not work by default.

I was pretty sure it worked with nightly.443 🤔 but I gave up on parcel because of how unstable it is at the moment and instead defaulted to webpack which is working really well

@albertwoo
Copy link

I am using 0.9.0 for react-refresh too.

I may try again when parcel 2 production version is released.

@Zaid-Ajaj
Copy link
Owner

@albertwoo Can you give the following a try and see if it fixes the problem?

Inside of ./www/fablejs/.fable/Feliz.1.17.2/Interop.fs change the source of reactElement function from

[<RequireQualifiedAccess>]
module Interop =
    let reactElement (name: string) (props: 'a) : ReactElement = import "createElement" "react"

to

[<RequireQualifiedAccess>]
module Interop =
    let inline reactElement (name: string) (props: 'a) : ReactElement = import "createElement" "react"

Added inline to the definition

And see if that fixes the import and uses createElement directly instead of a library function

@albertwoo
Copy link

Edit ./www/fablejs/.fable/Feliz.1.17.2/Interop.fs does not trigger fable to recompile. I may need to look into further.
Another strange thing I noticed that is under the folder fable of Feliz nuget pacakage there are generated .fs.js for every .fs files. Is it right. Because I noticed other package did not have those.

@Zaid-Ajaj
Copy link
Owner

Is it right. Because I noticed other package did not have those.

This is correct, Fable compiles the sources of the packages. About my suggested, fix, please ignore it because it doesn't work. I just tested it out 😓

@albertwoo
Copy link

So this is meant to be?
image

@Zaid-Ajaj
Copy link
Owner

So this is meant to be?

I think some .fs files don't have an equivalent .fs.js file because they aren't used. Which makes sense for Fable.React because Feliz only uses an alias of a type definition from there 😄

@albertwoo
Copy link

Ok. It is confuse for me because I thought that library author only need to put .fs file in the nuget package.

@Zaid-Ajaj
Copy link
Owner

Yes, library authors only put F# source code in there, Fable compiles those to JS module and puts them relative to where the F# source code was placed.

@albertwoo
Copy link

I think fable will compile those file under our own project folder right? My screenshot shows that under nuget cache folder the Feliz package itself already have those .fs.js files which should not be there. And I see the Feliz.fsproj copied all .js files. I think it should exclude *.fs.js:

<Content Include="*.fsproj; *.fs; *.js;" PackagePath="fable\" />

Any way it does not impact too much.

@Zaid-Ajaj
Copy link
Owner

@albertwoo Since Feliz v1.18 (now v1.19) the output of Feliz uses createElement directly without an intermediate reactElement function call in-between. Can you give it a try and see if it fixes your issue?

@albertwoo
Copy link

@Zaid-Ajaj just tried, it works perfect 👍👍👍

@MangelMaxime
Copy link
Contributor

MangelMaxime commented Jan 5, 2021

I am working on an experimentation using Feliz and wanted to test Fast Refresh and I wanted to share my experience as I spent my evening on it yesterday.

The support for Fast Refresh is done in majority by the bundler tools which have can their own implementations.

  • webpack can support it via react-refresh-webpack-plugin. However, the injected by this plugin will only trigger a refresh/HMR call (yes that's still what is used under the hood) if all the export from the current module are React components.

This cause problem when using Elmish still for the application because we always expose at least 2 functions update and init. Also the type reflection/definition generated by Fable etc. I didn't count the view because it is possible to use function component which is a React component 😉

  • snowpack in the other hand doesn't check what is exported by the module and always accept the HMR.

  • I also made some test manually using fast-refresh/runtime package and it confirm my point above.

To summarize, if you use Webpack you need to make all your funtion/type private in your file in order to have Fast Refresh support. If you use snowpack, you can do as you please.

However, my experience with snownpack experience not so great, I often have the file in the browser out of sync with what is on my disk. And also I don't find it robust enough yet to be used in production because I often end up tweaking the config or stop/restart the snowpack process.

To continue my test I will fork the react plugin and remove the "export check" and see how it goes.

@Shmew
Copy link
Collaborator Author

Shmew commented Mar 2, 2021

@Zaid-Ajaj are we good to close this issue?

@Zaid-Ajaj
Copy link
Owner

Zaid-Ajaj commented Mar 2, 2021

@Shmew I haven't gotten into it yet but I wanted to explore what @MangelMaxime has been doing. I think his solution might prove better suited for Feliz apps because each value and function are automatically exported. Maybe we should start a new issue from here, but let's not close this yet 🙏

@MangelMaxime
Copy link
Contributor

Just for your information, in the end I decided to go with standard a Elmish application using the old Elmish.HMR NuGet.

I encountered several issues (sorry it is a long time ago so I don't really remember... but it was probably related to HMR) when trying to go full Feliz way using Feliz.UseElmish that has been solved in Elmish.

I think the problem, is that fast-refresh only reload the components that has been updated and so the dispatch instance is not updated/propagated correctly or something like that.

Again, my tests are 2 months old and I don't precisely remember what was the problem.

Also because I don't write my application using "classical react components" but using "standard" Elmish way I mostly use Feliz for the API.

@roboz0r
Copy link
Contributor

roboz0r commented Mar 14, 2021

Could the issue with react-refresh-webpack-plugin requiring react components to be the only exports in the file be resolved by making the ReactComponentAttribute tell Fable to compile that function as a separate file i.e. Counter.fs becomes Counter.fs.js and CounterRC.fs.js or some suitable name?

@Zaid-Ajaj
Copy link
Owner

Let's close this one and if there are any concrete issues lingering, we discuss them in a new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants