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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fable support for Feliz/React applications #150

Closed
Zaid-Ajaj opened this issue Jul 20, 2020 · 23 comments
Closed

Fable support for Feliz/React applications #150

Zaid-Ajaj opened this issue Jul 20, 2020 · 23 comments

Comments

@Zaid-Ajaj
Copy link

Zaid-Ajaj commented Jul 20, 2020

Hi there,

For a while now, we have been trying to integrate the react-refresh plugin into a React application with the Fable. I tried of bunch of configuration and non have worked so far 馃槥 I am really hoping you could give us guidelines as to how to make the integration work. I realize that you most likely aren't familiar with Fable and its tool chain, so let me briefly explain how it works and what the problem is.

Fable integrates into webpack pretty much the same way typescript does with a specialized loader called fable-loader, this loader generated Babel AST and is then handed off to webpack for bundling and minification. To build React applications, we use a binding library called Feliz which offers an API and glue code that internally calls React APIs. Feliz code looks a lot like its React equivalent, for example:

import ReactDOM from 'react-dom'
import React, { useState } from 'react';

function Counter() {
  // Declare a new state variable, which we'll call "count"
  const [count, setCount] = useState(0);

  return (
    <div>
      <h1>{count}</h1>
      <button onClick={() => setCount(count + 1)}>
        Increment
      </button>
    </div>
  );
}

ReactDOM.render(<Counter />, document.getElementById("root"))

Translates to this F# code

module App

open Feliz

let counter = React.functionComponent(fun () ->
    let (count, setCount) = React.useState(0)
    Html.div [
        Html.h1 count
        Html.button [
            prop.text "Increment"
            prop.onClick (fun _ -> setCount(count + 1))
        ]
    ])

ReactDOM.render(counter, document.getElementById "root")

However, I am guessing that the code generated by Fable and especially by Feliz is something that this plugin cannot detect changes in for some reason. This is why I created a reproduction sample to demonstrate the issue

https://github.com/Zaid-Ajaj/react-fast-refresh-repro

UPDATE: the repository now includes the generated JS code (Babel AST before minification or bundling) within the directory called compiled-js which you can inspect without having to run or build anything

This repository includes a simplified React application built with Feliz and can be compiled with Fable. It uses webpack and it includes this plugin. To run application you simply execute npm install and npm start but unfortunately you need dotnet 3.1+ installed on your system

If it is not an option for you install dotnet to try out the sample, please let me know and I will figure something out

Now when we are running the application using npm start and we make changes to the file in src/App.fs where there is a simple counter function component, the changes are not detected and instead the whole page is refreshed. We are not sure why this is happening and we could use some advice about our generated code. It could also be something silly we overlooked in the configuration of the plugin.

I added a npm script called debug-compiled-js which compiles the application into its intermediate state as the output of the Babel AST before it is handed off to webpack, you can run this script using npm run debug-compiled-js and you will see the generated App.js file in the compiled-js directory.

I would really really appreciate it if you could take a look 馃檹 see if we did the configuration the right way and whether we should change or simplify the way our generated JS looks like so that this plugin and future iterations of it can work with Fable.

Thanks a lot in advance 鉂わ笍

Zaid

@gaearon
Copy link

gaearon commented Jul 20, 2020

From a cursory look, there's two missing pieces. Our Babel plugin needs to be able to detect React components (which it does by PascalCase function naming convention) and calls to Hooks (which it does by tracking use* function calls).

In this example, both of these aspects are obscured:

var counter = (0, _React.React$$$functionComponent$$2F9D7239)(function () {
  var xs, properties, xs$$1, properties$$2;
  var patternInput = (0, _React.Feliz$002EReact$$React$002EuseState$002EStatic$$1505)(0);
  // ...
});

You could probably write an alternative Babel plugin which would detect $002Euse* as a Hook pattern and functions inside React$$$functionComponent as function components. Or find a way to bring the output closer to the heuristic used by our Babel plugin (which would also give you the benefit of making the output more readable).

@Zaid-Ajaj
Copy link
Author

Hi @gaearon, thanks a lot the suggestions! I was able to simplify the generated code by removing some layers between Feliz and the raw React function calls. Using this F# snippet:

open Fable.Core.JsInterop

let useState(value: 't) = import "useState" "react"

let Counter() =
    let (count, setCount) = useState(0)
    Html.div [
        Html.button [
            prop.style [ style.marginRight 5 ]
            prop.onClick (fun _ -> setCount(count + 1))
            prop.text "Increment"
        ]

        Html.button [
            prop.style [ style.marginLeft 5 ]
            prop.onClick (fun _ -> setCount(count - 1))
            prop.text "Decrement"
        ]

        Html.h1 count
    ]

The generated code now looks like this:

var _react = require("react");

var useState = _react.useState;

exports.useState = useState;

function Counter() {
  var xs, properties, xs$$1, properties$$2;
  var patternInput = useState(0);
  var children = (0, _List.ofArray)([(xs = (0, _List.ofArray)([(properties = new _Types.List((0, _Interop.mkStyle)("marginRight", 5), new _Types.List()), (0, _Interop.mkAttr)("style", (0, _Util.createObj)(properties, 0))), (0, _Interop.mkAttr)("onClick", function handler(_arg1) {
    patternInput[1](patternInput[0] + 1);
  }), (0, _Interop.mkAttr)("children", "Increment")]), (0, _Interop.reactElement)("button", (0, _Util.createObj)(xs, 0))), (xs$$1 = (0, _List.ofArray)([(properties$$2 = new _Types.List((0, _Interop.mkStyle)("marginLeft", 5), new _Types.List()), (0, _Interop.mkAttr)("style", (0, _Util.createObj)(properties$$2, 0))), (0, _Interop.mkAttr)("onClick", function handler$$1(_arg2) {
    patternInput[1](patternInput[0] - 1);
  }), (0, _Interop.mkAttr)("children", "Decrement")]), (0, _Interop.reactElement)("button", (0, _Util.createObj)(xs$$1, 0))), (0, _Interop.reactElement)("h1", {
    children: new Int32Array([patternInput[0]])
  })]);
  return (0, _Interop.reactElement)("div", {
    children: _Interop.reactApi.Children.toArray(children)
  });
}

(0, _ReactDOM.ReactDOM$$$render$$Z3D10464)(function () {
  return Counter();
}, document.getElementById("feliz-app"));

The plugin now seems to be able to detect React patterns in the code (function components and hook usages). However, it doesn't show the application anymore and instead shows an error page as shown below

2020-07-20 19_20_01-Feliz App

At least that is progress and very valuable information about the generated code! We will look into how we should optimize the output Javascript coming from Fable to match what the plugin expects.

Any ideas on what we should change next to make application show up on screen? Again, thanks a lot for your time, really appreciate it 馃檹 鉂わ笍

@Zaid-Ajaj
Copy link
Author

Alright, I managed to get the application running again. It seems that I cannot give a function as a direct argument for the render function from react-dom but instead I needed to create a ReactElement out of it first by calling createElement(Counter, { }) and now the generated code like this:

var useState = _react.useState;
exports.useState = useState;
var createElement = _react.createElement;
exports.createElement = createElement;
var renderDOM = _reactDom.render;
exports.renderDOM = renderDOM;

function Counter() {
  var xs, properties, xs$$1, properties$$2;
  var patternInput = useState(0);
  var children = (0, _List.ofArray)([(xs = (0, _List.ofArray)([(properties = new _Types.List((0, _Interop.mkStyle)("marginRight", 5), new _Types.List()), (0, _Interop.mkAttr)("style", (0, _Util.createObj)(properties, 0))), (0, _Interop.mkAttr)("onClick", function handler(_arg1) {
    patternInput[1](patternInput[0] + 1);
  }), (0, _Interop.mkAttr)("children", "Increment")]), (0, _Interop.reactElement)("button", (0, _Util.createObj)(xs, 0))), (xs$$1 = (0, _List.ofArray)([(properties$$2 = new _Types.List((0, _Interop.mkStyle)("marginLeft", 5), new _Types.List()), (0, _Interop.mkAttr)("style", (0, _Util.createObj)(properties$$2, 0))), (0, _Interop.mkAttr)("onClick", function handler$$1(_arg2) {
    patternInput[1](patternInput[0] - 1);
  }), (0, _Interop.mkAttr)("children", "Decrement")]), (0, _Interop.reactElement)("button", (0, _Util.createObj)(xs$$1, 0))), (0, _Interop.reactElement)("h1", {
    children: new Int32Array([patternInput[0]])
  })]);
  return (0, _Interop.reactElement)("div", {
    children: _Interop.reactApi.Children.toArray(children)
  });
}

renderDOM(createElement(function () {
  return Counter();
}, {}), document.getElementById("feliz-app"));

The application works nice but unfortunately still does a full page refresh when changing the content of the function (for example changing "Inrement" to "Increment!!!"

@gaearon
Copy link

gaearon commented Jul 20, 2020

createElement(function () {
return Counter();
},

This doesn't look right.

Should be this?

renderDOM(createElement(Counter, {}), document.getElementById("feliz-app"));

Also, renderDOM call should be in another file. Your component should be exported.

@Zaid-Ajaj
Copy link
Author

Your component should be exported.

Unfortunately, no dice 馃槥 after using separate files (from which the counter component is exported, now called App) The exports used are following this syntax:

exports.App = App;

function App() {
  var xs, properties, xs$$1, properties$$2;
  var patternInput = useState(0);
  var children = (0, _List.ofArray)([(xs = (0, _List.ofArray)([(properties = new _Types.List((0, _Interop.mkStyle)("marginRight", 5), new _Types.List()), (0, _Interop.mkAttr)("style", (0, _Util.createObj)(properties, 0))), (0, _Interop.mkAttr)("onClick", function handler(_arg1) {
    patternInput[1](patternInput[0] + 1);
  }), (0, _Interop.mkAttr)("children", "Increment!")]), (0, _Interop.reactElement)("button", (0, _Util.createObj)(xs, 0))), (xs$$1 = (0, _List.ofArray)([(properties$$2 = new _Types.List((0, _Interop.mkStyle)("marginLeft", 5), new _Types.List()), (0, _Interop.mkAttr)("style", (0, _Util.createObj)(properties$$2, 0))), (0, _Interop.mkAttr)("onClick", function handler$$1(_arg2) {
    patternInput[1](patternInput[0] - 1);
  }), (0, _Interop.mkAttr)("children", "Decrement")]), (0, _Interop.reactElement)("button", (0, _Util.createObj)(xs$$1, 0))), (0, _Interop.reactElement)("h1", {
    children: new Int32Array([patternInput[0]])
  })]);
  return (0, _Interop.reactElement)("div", {
    children: _Interop.reactApi.Children.toArray(children)
  });
}

Could it be that only export function ... is supported?

Should be this? renderDOM(createElement(Counter, {}), document.getElementById("feliz-app"));

Probably, right now I don't have a why of generating code that looks like this (this is how Fable currently does things) I can discuss it with the maintainers to see if we can generate simplified code without the extra function call.

In any case, do you have any pointer for me to look into how this plugin tries to detect the react components? I think it could be nice if I can fiddle with this plugin locally and debug it (putting console.log statements here and there) though I might be too optimistic

@gaearon
Copy link

gaearon commented Jul 20, 2020

The latest version of the code looks all right to me. Can you make a version of your repro with compiled code for this version?

@Zaid-Ajaj
Copy link
Author

Just published the modified F# code and its corresponding Javascript output. You should see now two files ./compiled-js/Components.js and ./compiled-js/App.js where a Counter function is exported from Components.js and used from App.js

@gaearon
Copy link

gaearon commented Jul 20, 2020

I'm getting this on yarn start:

ERROR in ./src/App.fsproj
Module build failed (from ./node_modules/fable-loader/index.js):
Error: write EPIPE
    at /Users/gaearon/p/react-fast-refresh-repro/node_modules/fable-loader/index.js:147:18
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
 @ multi ./node_modules/@pmmmwh/react-refresh-webpack-plugin/client/ReactRefreshEntry.js ./node_modules/@pmmmwh/react-refresh-webpack-plugin/client/ErrorOverlayEntry.js ./src/App.fsproj app[2]

Is this because I haven't installed dotnet? I'd like to run your version without compilation.

@gaearon
Copy link

gaearon commented Jul 20, 2020

All right I got it running.

@gaearon
Copy link

gaearon commented Jul 20, 2020

Probably, right now I don't have a why of generating code that looks like this (this is how Fable currently does things) I can discuss it with the maintainers to see if we can generate simplified code without the extra function call.

This still seems like a problem. From React's point of view, you're not rendering the App component at all. You're rendering a function component of a different type (which happens to call the App function inside). I don't know if that's the source of the problem or not, but we need to get from createElement(function () { App() } } to createElement(App).

@gaearon
Copy link

gaearon commented Jul 20, 2020

So far the furthest I got was by keeping

let Counter = React.functionComponent(fun () ->
    let (count, setCount) = useState(0)
    Html.div [
        Html.button [
            prop.style [ style.marginRight 5 ]
            prop.onClick (fun _ -> setCount(count + 1))
            prop.text "Increment!"
        ]

        Html.button [
            prop.style [ style.marginLeft 5 ]
            prop.onClick (fun _ -> setCount(count - 1))
            prop.text "Decrement"
        ]

        Html.h1 count
    ]
)

in one file and

module App

open Feliz
open Browser.Dom
open Fable.Core
open Fable.Core.JsInterop

let createElement (value: obj) : obj = import "createElement" "react"
let renderDOM x y = import "render" "react-dom"

renderDOM (createElement Components.Counter) (document.getElementById "feliz-app")

in the other file. This still has the naming issue but it might actually not be a problem.

The problem I'm seeing is that $Refresh$.setup() only seems to be called for a few modules:

Screenshot 2020-07-20 at 20 44 46

Screenshot 2020-07-20 at 20 44 58

It isn't being called for our Components file, which is why registration doesn't work. I would recommend to look into why those setup() calls are happening for some modules but not the others.

@Zaid-Ajaj
Copy link
Author

From React's point of view, you're not rendering the App component at all. You're rendering a function component of a different type (which happens to call the App function inside)

That makes sense. I managed to get the compiled code to generate the createElement call without creating an intermediate function in between: introducing a JS file (which I called Bridge.js) that imports the counter and exports it as is, then importing that file from App.js to the desired output:

renderDOM(createElement(Counter, {}), document.getElementById("feliz-app"));

You can see and pull the changes for the repo in its current state. The code is nowhere close to usable right now but at least we can potentially track down the problem (we can always change how Fable does things or at least build extension points that allow more custom generated code)

@Zaid-Ajaj
Copy link
Author

The problem I'm seeing is that $Refresh$.setup() only seems to be called for a few modules ... It isn't being called for our Components file, which is why registration doesn't work

Hmmm interesting, I had the feeling that it was in fact detecting the components because when I used render incorrectly, I got the error screen which is coming from the plugin. Though that might be something global when any exception is thrown and nothing has catched it. I will look into it, I assume this function is coming from the plugin itself

@gaearon
Copy link

gaearon commented Jul 20, 2020

Ok I got a bit further. Is there a way to make let Counter = React.functionComponent(fun () -> give a name to the inner function?

@Zaid-Ajaj
Copy link
Author

Is there a way to make let Counter = React.functionComponent(fun () -> give a name to the inner function?

I am afraid there isn't in Fable/F# 馃槥 fun () -> always translates to an anonymous function. Unless you write the function the way you want it from the Bridge.js file in JS and then import it from App.fs before passing it down to render

@gaearon
Copy link

gaearon commented Jul 20, 2020

What is it exactly that causes the wrapping at createElement callsite? This seems highly dangerous in general because it destroys all local state below in the tree. Why is it introducing this semantic difference?

@gaearon
Copy link

gaearon commented Jul 20, 2020

Here's the fix based on your latest master.

diff --git a/src/Components.fs b/src/Components.fs
index 4b22eeb..dd6ba07 100644
--- a/src/Components.fs
+++ b/src/Components.fs
@@ -3,15 +3,24 @@ module Components
 open Feliz
 open Fable.Core.JsInterop
 
-let useState(value: 't) = import "useState" "react"
+// Prevent it from leaking into exports.
+// If it leaks into exports, we'll think this file exports
+// something more than React components, so we'll not be able
+// to refresh this file without going upwards in require chain.
+let private useState(value: 't) = import "useState" "react"
 
-let Counter() =
+// Export a named function. In this case the compiler output
+// makes it anonymous but thankfully ES6 implicit naming for
+// function expression in assignment does the job.
+// Without a name, we wouldn't know this is a React component,
+// and wouldn't be able to update this file independently.
+let Counter = fun () ->
     let (count, setCount) = useState(0)
     Html.div [
         Html.button [
             prop.style [ style.marginRight 5 ]
             prop.onClick (fun _ -> setCount(count + 1))
-            prop.text "Increment!"
+            prop.text "Increment"
         ]
 
         Html.button [
@@ -21,4 +30,4 @@ let Counter() =
         ]
 
         Html.h1 count
-    ]
\ No newline at end of file
+    ]
diff --git a/webpack.config.js b/webpack.config.js
index ca5cd70..f3a496b 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -101,7 +101,14 @@ module.exports = {
             new CopyWebpackPlugin([{ from: resolve(CONFIG.assetsDir) }]),
         ])
         : commonPlugins.concat([
-            new ReactRefreshWebpackPlugin(),
+            new ReactRefreshWebpackPlugin({
+                // All our components are written in FS.
+                // So only include that.
+                // Intentionally don't include JS because
+                // it's unnecessary and clutters the output
+                // for modules that aren't actually components.
+                include: /\.fs$/
+            }),
         ]),
     resolve: {
         // See https://github.com/fable-compiler/Fable/issues/1490

We could maybe add f to include array by default here so you wouldn't have to tweak plugin options.

@Zaid-Ajaj
Copy link
Author

Why is it introducing this semantic difference?

I believe (but I am not entirely sure) that the reasoning has to do with the fact that we are using a function that is written in F# as input for another function used in JS: functions written in F# are automatically curried so I think Fable uncurries the function at call site of external JS APIs to make them adhere to standard JS conventions while allowing F# devs to write functions the same way they are used to. At least that is how I remember it

let add x y = x + y
// translates to curried definition
const add = (x) => (y) => x + y
// uncurried when used in bindings/external JS code
consumeAdd(function (x, y) { 
  return add(x)(y);
})

In the case of fun () -> the currying/uncurrying might totally unnecessary but I am can't say for sure in all cases

@gaearon
Copy link

gaearon commented Jul 20, 2020

I guess really the fix is just adding internal to that let useState thing, and fixing the include option. The rest works.

But ideally you would never generate a passthrough function when there's no actual currying intended (such as when passing code into React JS APIs). Since that messes with function identity, and React cares about function identities a lot.

@Zaid-Ajaj
Copy link
Author

Zaid-Ajaj commented Jul 20, 2020

Oh my god it actually works 馃槏 鉂わ笍 I can't thank you enough for taking the time to actually debug the code 馃檹 I think that I understand how we should simplify the code gen in Fable to make it output more React-friendly code

  • Remember to include F# modules new ReactRefreshWebpackPlugin({ include: /\.(f|j)s$/ })
  • Function components are always PascalCase
  • Function components are always exported
  • Entry file where render is called should not contain function component definitions
  • Entry component should not be wrapped or otherwise you use the function stable identity/reference
  • let Counter = React.functionComponent(fun () -> complicates the situation. Use let Counter() = ... instead
  • Using function React.useState works without an extra simplified useState

fast-refresh-working

@gaearon
Copy link

gaearon commented Jul 20, 2020

Function components are always exported

I think it's more that if your module exports something that doesn't look like a function component (PascalCase function), we're going to have to look at all its "parent" modules instead, and continue this analysis up the tree. If we reach the root, we reload the whole app.

@Zaid-Ajaj
Copy link
Author

I see, that makes sense. In any case I believe we can achieve this requirement on the Fable side of things using a compile-time extension so that users don't have to bother with it really, so every annotated function component becomes pascal case by default if that isn't the case already 馃槃

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 22, 2020

Dan - thanks for stepping in while I was away busy with other stuff!

I think I'll add some FAQ to the README to clarify some of these underlying assumptions.

We could maybe add f to include array by default here so you wouldn't have to tweak plugin options.

I'm not sure about this.

  • Might make sense to only include .fs as you've shown to not allow any js files sneak in (explicitly)
  • F# is not a JS related ("superset", like TS/Flow are)
  • I'm not sure how many people it would benefit tbh ...

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

No branches or pull requests

3 participants