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

New API for FunctionComponent #188

Merged
merged 4 commits into from
Apr 21, 2020
Merged

New API for FunctionComponent #188

merged 4 commits into from
Apr 21, 2020

Conversation

alfonsogarciacaro
Copy link
Member

@alfonsogarciacaro alfonsogarciacaro commented Mar 30, 2020

TL;DR To use the new API, change the code as follows:

// Declaring site. Before:
let MyComponent = FunctionComponent.Of(fun (props: {| value: string |} -> (* view code *))

// Now (note `displayName` argument is not needed any more):
type MyComponent() =
  inherit FunctionComponent<{| value: string |}>(fun props -> (* view code *))

// Call site. Before:
MyComponent {| value = foo |}

// Now (use type alias if you find it too verbose: `type C = FunctionComponent`)
FunctionComponent.Render<MyComponent,_> {| value = foo |}

When React hooks were out and functions became the preferred way to define React components, we tried to use the opportunity to move away from the class components as the syntax was a bit messy and promote the use of React components, so the React development tools become more useful in Elmish apps.

However after working in several projects with the FunctionComponent helper I've noticed several problems.

  • It looks too similar to a simple helper function. This was originally intended to make it easier to use, but unfortunately it makes it much more difficult to see the difference between a React component (that can use hooks and is displayed in the dev tools) and a simple helper function.
  • The display name, which is necessary to identify the component in the dev tools, must be passed as a string argument and is easy to forget. When using a lambda the component just appears in the dev tools as "Anonymous".
  • Because of F# value restrictions, the compiler complains when using FunctionComponent with props that accept a generic argument. This makes it very cumbersome to use it to build reusable components.

This new proposal for the API solves this problems. Please check how it's applied in the diff for Samples/TodoMVC/src/TodoMVC.fs. This is kind of a breaking change, but I've tried to minimize the changes needed to update an existing code base and also I'm keeping the old Of method but marking it as deprecated.

screencast

@MangelMaxime
Copy link
Member

In Feliz, binding it seems like Zaid found a way to allow people to store the FunctionComponent directly in a let binding.

This means the functionComponent can be called directly without an extra helper like FunctionComponent.Render.

Example

Should/could we do the same? Or does this imply some limitation like being unable to provide the memoriWith arguments and don't solve the problem you mentioned in your message?

@alfonsogarciacaro
Copy link
Member Author

Yes, this is is basically the same as the current FunctionComponent.Of but unfortunately it's affected by the problems listed above ☝️ Using a helper to render the component instead of calling it directly as a function is intentional to make it more distinguishable from simple functions that don't become React components.

@MangelMaxime
Copy link
Member

Ok, I re-read the whole thread and I think I better understand your reasons.

Reason 2 and 3 are indeed justified IHMO.

About reason 1 I just want to add something about this part:

it makes it much more difficult to see the difference between a React component (that can use hooks and is displayed in the dev tools) and a simple helper function

IHMO it doesn't really matter when calling a component if it's a stateful component, stateful component, etc.

For example, in Fable REPL the fact that editor is a stateful component doesn't matter on calling side. We just want it to give us a ReactElement.


One more question, does it means that FunctionComponent created the new way are not compatible with calling them directly from JavaScript code?

If people want to expose the component they will have to expose:

let renderMyComponent props =
    FunctionComponent.Render<MyComponent,_> props

And not just

type MyComponent() =
    inherit FunctionComponent<{| value: string |}>(fun props -> (* view code *))

I am ok with the new API if we release as a new major version because it's a breaking change :)

Because we are releasing a new major version, I think we should take this opportunity to remove all the old APIs marked as Obsolete in order to clean a bit the codebase. What do you think?

@Zaid-Ajaj
Copy link
Member

Hello @alfonsogarciacaro, first of all I am really glad to see you are back! Hope you are doing well!

We have already discussed this before on the Feliz repo and I know you didn't like my approach to the problem. However, I don't believe this API will fix the problems you mentioned: being able to easily distinguish simple functions vs. function components will not necessarily promote the use of function components in Elmish applications. Especially given the fact that now you have to inherit from a class to build a "function" component which is weird to use from F#. I can see people opting for simple functions rather than function components because of how weird the API looks like.

This API will look very very different than the React API that people from JS are used to work with!

I have yet to an example where using function components everywhere helps make the application perform better. Yes, there are a number of places to use them (e.g. entry points of components) but I wouldn't want to use them everywhere because we will be exchanging the diffing costs with costs of equality checks using equalsButFunctions.

Because of F# value restrictions, the compiler complains when using FunctionComponent with props that accept a generic argument. This makes it very cumbersome to use it to build reusable components.

Using a generic function component is really easy, why is it cumbersome?

type GenericList<'T> = {
    Values: 'T list
    Render : 'T -> ReactElement
}

let genericComponent<'T> = React.functionComponent(fun (props: GenericList<'T>) ->
    React.fragment [
        for value in props.Values do
            props.Render value
    ])

let ui = genericComponent { 
    Values = [1 .. 10]
    Render = fun x -> Html.h1 x 
}

Of course, it is all up to you what you want to do with this API, I just wanted to point out that it will also have downsides that might have overlooked.

@alfonsogarciacaro
Copy link
Member Author

Hi @MangelMaxime and @Zaid-Ajaj! I'm fine, thanks, still with little time left for open source but at least Japan is not taking (for now) the drastic measures that are happening in Europe. I hope you're all well!

Unfortunately the generic values in F# are just syntactic sugar for functions in disguise (this is needed, because let test<'T> = typeof<'T> gives different values depending on how you call it) and are not eagerly calculated. This can lead to subtle bugs, for example when trying to memoize or use a run-once effect with generic components, as you can see in this example.

screencast

This gets fixes with the new API. However, I'm not very concerned about the performance with memo. In fact, after applying it to several components I had several situations where the component didn't re-render when I expected, and apply memo much more sparsely now.

My main concern is about the maintainability of big apps. In React apps developers use components by default, because JSX is just syntax sugar to initialize a component instead of just calling a function. Thanks to this, developers can take advantage of React Dev Tools and easily locate and/or profile a component when necessary. In Elmish apps, by default developers use simple functions to build the app which makes the React Dev Tools less useful. In small apps it doesn't really matter but as the app grows in size and complexity it does.

It has happened to me several times already when I have to go back to an existing app to modify something that finding the right code becomes tricky (I've to admit we went a bit crazy with abstractions and higher-order functions in this project) and I've also spent several days to find a performance bottleneck that could have been spotted easily with the React profiler. When the app is big it's more difficult to start turning chunks of it into components because it requires a lot of changes, it's much more efficient to do it from the beginning (as React is designed to).

This is why I'd like devs to get used to call something like FunctionComponent.Render<MyComponent,_> props because it makes it clear that we're instantiating a component and not just calling a function (we can change the name if it's too verbose or if the semantics can be improved). You're right that using a class for a "function" component is misleading, but I tried different approaches and this was the only one I could find that fixed the problems mentioned above and it was sufficiently distinct from simple function calls.

@Zaid-Ajaj
Copy link
Member

this is needed, because let test<'T> = typeof<'T> gives different values depending on how you call it

Question, does this have to do with the fact that function components have to defined once on as module-level value so that the internal state and effects are not reset every time you call them as described in Common Pitfalls?

if that is the case, then the solution is really simple: defining the concrete value of the generic component, also as module level value, and using that one instead:

type GenericList<'T> = {
    Values: 'T list
    Render : 'T -> ReactElement
}

// generic version
let genericComponent<'T> = React.functionComponent(fun (props: GenericList<'T>) ->
    React.fragment [
        for value in props.Values do
            props.Render value
    ])

// concrete version (must be at module level) 
let concreteComponentOfIntList = genericComponent<int list>

let render state dispatch = 
  concreteComponentOfIntList {
    Values = [1 .. 10]
    Render = fun x -> Html.h1 x 
  }

You can argue that it is not really straightforward for F# developers to apply this restriction to their code and that it can be easily forgotten. This ties to what you were saying

this was the only one I could find that fixed the problems mentioned above

This is not the only solution and I believe that we can solve by providing better tooling for Elmish/React applications: I am planning on building a F# analyzer for Feliz/React which should be able to detect incorrect usages of React function components and advise users on how to fix these issues (moving concrete definitions to a module-level value) along with giving suggestions like including the display name when it is not provided, missing keys when looping over lists and rendering them and finally detecting incorrect combinations of HTML attributes. The analyzer solves many of these issues I believe.

A bit off topic but relevant to issues of function components: Fable.Elmish.HMR doesn't play well with them and internal state changes are reset when modifying the code cc @MangelMaxime (I will be able to make a repro soon)

@alfonsogarciacaro
Copy link
Member Author

Yes, it has to do with that pitfall and thanks for the reminder: we can add this to the list of benefits of the new API, as it's not possible to define it within another function by mistake :)

And also yes, that's the way to avoid the pitfall. The main issue is it puts the burden on the consumer as most of the times you don't know what will replace the generic parameter. And the first impulse of most of the users would be to call genericComponent<int list> directly without creating the concrete version. (The boilerplate also starts to get bigger when you build a generic component out of other generic components.)

I admit that it looks weird to use a class for a "function" component, and that we've changed the API to create React components too many times already. But after my experience in different real-world scenarios, I still believe this API (or something along these lines) solves many of the issues without having to build and maintain specific tooling for it. But if there's no value in adding it to the base library I'm ok with keeping it apart and using it by just adding a reference to the file, which I'm already happily doing in my projects :)

@Zaid-Ajaj
Copy link
Member

I still believe this API (or something along these lines) solves many of the issues without having to build and maintain specific tooling for it

It is always a bit of a balance between correctness, type-safety and lightweight syntax. Personally, I would always choose the former because I know it will benefit everyone in the long run. However, in this case I very skeptic about introducing yet another syntax for function components in the base library that IMHO is counter-intuitive.

Of course, you have written more Fable applications than I did and you have a lot more experience so you know better. These were my 2 cents on the subject matter as I am thinking about how I would explain this to newcomers to Fable from F#/JS and to the existing userbase.

@Luiz-Monad
Copy link
Contributor

Luiz-Monad commented Apr 9, 2020

I usually know that my functions are components because of this happening, forwardref denounces it, all of them are ReactElementType too.

 let private childFn = forwardRef <| fun (props: ChildProps<IItem>) ref ->
     ///impl ommited

 ReactElementType.create childFn {
                        state = state
                        config = config
                    } []

I wouldn't mind changing it because my components are in their own folder, so I know they're components when I look at the path, but the type must show it.

But I find it funny having to use inheritance in functional programs.

@Luiz-Monad
Copy link
Contributor

Luiz-Monad commented Apr 9, 2020

It looks too similar to a simple helper function. This was originally intended to make it easier to use, but unfortunately it makes it much more difficult to see the difference between a React component (that can use hooks and is displayed in the dev tools) and a simple helper function.

That's what happen when your function is not in fact pure, hooks make pure functions impure. That's a conceptual problem with React and isn't solvable without changing their API.

let fnWithHook props = 
     let (readState, setState)  = HookBindings.Hooks.useState props.initValue
     ///ommited

Is in fact, as if we had a global hook:

let fnWithHook hook props = 
     let (readState, setState)  = hook.useState props.initValue
     ///ommited

I don't know if there's a good solution, programming language research is being done on dependent types for those kind of problems. Like getting rid of hook state out of the function in a better and safe way, or getting that pesky dispatch lambda out of the argument list. (that would happen in the type level without having to change any syntax, it would flow with the type, the moment you used a hook it would mark the function as hooked in some kind of tag in the type, like a mutable/immutable tag we already have on some types)

In fact, all hooks are about side effects, you can't use side effects and be pure and pretend that the function is a normal pure function, a component is not a function, and there's no such thing as "PureFunctionComponent", this is kind of a lie. If it was to be truly pure then it must not be possible to have any hooks in its body, that kind of defeats its usefulness.

Perhaps it would be best for ReactElement to be a monad like the IO monad, so it would be explicit and functional.
The simpler way would be just "boxing" functions meant to be components and changing the React.render signature so its composable and provides the correct result when the computation is yielded.

@Luiz-Monad
Copy link
Contributor

Luiz-Monad commented Apr 9, 2020

So I propose something the following pattern (monads are kind of a pattern for functional programming).

open Fable.React

type Delay<'props> = Delayed of (unit -> ReactElementType<'props>)
//I
type ReactComponent () =
    member m.Return f = ReactElementType.ofFunction f
    member m.Delay f  = Delayed f  //III
    member m.Run ( Delayed f ) = 
        let fl = lazy ( f () ) //II
        fun props -> ReactElementType.create fl.Value props []
let reactComp = ReactComponent ()

Usage would be:

let componentFunction = reactComp {
    return (fun ( props: seq<Props.IHTMLProp> ) -> div props [])
}

(I) As we now have a monad type, we can capture and know for sure we're dealing with a component.

It looks too similar to a simple helper function. This was originally intended to make it easier to use, but unfortunately it makes it much more difficult to see the difference between a React component (that can use hooks and is displayed in the dev tools) and a simple helper function.

(II) Display name could be applied and it would work correctly, and the programmer would never forget to do it.

The display name, which is necessary to identify the component in the dev tools, must be passed as a string argument and is easy to forget. When using a lambda the component just appears in the dev tools as "Anonymous".

(III) Because the way computation expressions work it solves the problem with the type inference. (I think they're converted to a member call to return/bind/etc and member calls have a different kind of dispatching than normal functions ). And because we create a monad we can compose functions (combine/bind implementation is left as an exercise)

Because of F# value restrictions, the compiler complains when using FunctionComponent with props that accept a generic argument. This makes it very cumbersome to use it to build reusable components.

@alfonsogarciacaro
Copy link
Member Author

That's a great idea @Luiz-Monad! It could be used to convey the idea that hooks are hidden side-effects, and it also has the merit many F# developers like CEs so they may be more willing to accept this kind of API.

Only issue is in in my quick tests, it seems we still need generic values (functions in disguise) for generic components which would recreate the component every time (see the compiled JS code in this sample). I guess we could use a cache to create the component only the first time the CE is run, but I know that I think about it we can do the same to fix the current API 🤔

@MangelMaxime
Copy link
Member

@alfonsogarciacaro Just a remarks, be careful about the cache. I am not sure how it will react to HMR calls.

If that's broken on HMR call then you need to learn to your cache how to work with it.

You can check one of my explanation here

@alfonsogarciacaro
Copy link
Member Author

@MangelMaxime You're right. I just realized this proposal was not compatible with HMR. Fortunately there's a callback to get notified of new HMR updates and then clear the cache. With this commit HMR works again. Maybe we can use a similar trick for the equality functions.

Later I will try to apply the cache also to the current FunctionComponent.Of API to see if we can solve the problem of the component being recreated every time for generic values without breaking changes.

@MangelMaxime
Copy link
Member

MangelMaxime commented Apr 16, 2020

Indeed, if your handler works for equality function yes.

Otherwise, I had this implementation available:

module HMR =

    let hot = HMR.``module``.hot

    /// Normal structural F# comparison, but ignores top-level functions (e.g. Elmish dispatch).
    /// Can be used e.g. with the `FunctionComponent.Of` `memoizeWith` parameter.
    let equalsButFunctions (x: 'a) (y: 'a) =
        #if DEBUG
            if hot.status() = HMR.Status.Apply then
                false
            else
                Fable.React.Helpers.equalsButFunctions x y
        #else
            Fable.React.Helpers.equalsButFunctions
        #endif

Coming from elmish/hmr#26

It's better to make equality functions support HMR out of the box. It avoids the needed for people to remember to open Elmish.HMR module after their open Fable.React line.

@alfonsogarciacaro alfonsogarciacaro force-pushed the new-function-component branch 2 times, most recently from 1d80228 to 9a3c4a9 Compare April 17, 2020 12:26
@alfonsogarciacaro
Copy link
Member Author

OK, I've changed the PR so it solves two of the problems mentioned above (forgotten displayName and component being recreated when called within a function) with no breaking changes! 🎉

Another benefit of using the cache is now it's easier to capture the dispatch function the first time (I think that's ok, not sure if we had a discussion about it) and use the model as props:

let MyView model dispatch =
    model |> FunctionComponent.Of(fun (props: Model) ->
      // Use dispatch here, as it was captured the first time the function was called

@MangelMaxime I think we can wrap the memoizeWith argument to add the HMR check (so it works with any equality function) but in my tests the memoized components did actually refresh after HMR. Do you have an example that I can use to reproduce the issue?

@MangelMaxime
Copy link
Member

MangelMaxime commented Apr 20, 2020

@alfonsogarciacaro I think my problem was specific to equalsButFunctions.

I am passing the dispatch function as a prop because I need to send a message from a FunctionalComponent. The problem is that when HMR trigger we start a new Elmish instance so dispatch reference needs to changes.

But because we ignore the function checks in equalsButFunctions then it is not updated.

The only reproduction, I have right now is fulma-demo on branch inbox.

Edit: I didn't review yet your latest change. I will probably do it this evening or tomorrow.

If you still need a cleaner/smaller reproduction please ask me I will try to provide one.

@alfonsogarciacaro
Copy link
Member Author

Thanks @MangelMaxime! Hmm, I tested the inbox branch of fulma-demo (the mail app is super cool by the way). I replaced the HMR.equalsButFunctions to use the version from Fable.React, but when I do any change everything seems to be working normally 🤔

screencast

I am passing the dispatch function as a prop because I need to send a message from a FunctionalComponent. The problem is that when HMR trigger we start a new Elmish instance so dispatch reference needs to changes.

With this PR, even if you capture dispatch instead of passing it as a prop, it should work with HMR. Because the component is recreated after each HMR so the dispatch function is recaptured again.

@Zaid-Ajaj
Copy link
Member

OK, I've changed the PR so it solves two of the problems mentioned above (forgotten displayName and component being recreated when called within a function) with no breaking changes!

This is awesome @alfonsogarciacaro ❤️

@MangelMaxime
Copy link
Member

the mail app is super cool by the way

Thank you :)

If everything works for you them that's perfect. I am going to release the new version of Fable.React

@MangelMaxime MangelMaxime merged commit 81ca109 into master Apr 21, 2020
@alexswan10k
Copy link

alexswan10k commented Apr 24, 2020

Hi Guys,

I have hit an issue with the new caching mechanism that may be worth noting:

In my current project I have some code that does some slightly naughty/hacky inline function components, the key detail being that they close over variables from a higher scope. As it stands there are probably a bunch of reasons why I should fix this, but the point I wanted to make is that this isnt entirely intuitive from a users point of view that only the variables on the first pass are closed over.

My code looks something roughly like the following (its basically a function that dynamically generates a graph of function component definitions from data)

let rec complicatedRecFn threadedThing = function
    | A x->
        FunctionComponent.Of(fun props ->
            div [] [someOtherFunctionComponent(threadedThing.value, x)]
            )
    | B children ->
        div [] [
            yield! children |> List.map (complicatedRecFn { threadedThing with SomethingElse = blah })
        ]
    | C -> ...

Other than 'fix it' :) Any thoughs on what best to do here?

Beyond this though, absolutely loving Fable etc, great job so far.

@MangelMaxime
Copy link
Member

Thank you for the kind words and really happy that you like this project :)

I am not sure if my answer falls into the "fix it" category but in theory, you should pass variables used by a component via the props.

Because doing so, allows React diffing mechanism to works. In the past, you didn't have the problem because it was re-creating the components and the views from scratch as if it never existed at first. This was because of a limitation in our previous implementation and the current one is now respecting what React expect in theory.

Sorry for the vague answer, I hope it helps you a bit.

@alexswan10k
Copy link

Yeah I appreciate where you are coming from, my situation is a bit weird because I am using an external tree to generate the react component hierarchy, so there are things that get injected in 'before' the tree is handed to react and starts rendering reactively (with all the memoization tricks etc).

Its also fair to point out that this is kinda consistent with how react effect hooks work - an inline function that gets captured once and then cached.

I will roll back for now, and fix/refactor my end when I get a chance :)

@MangelMaxime MangelMaxime deleted the new-function-component branch April 26, 2020 12:58
@alfonsogarciacaro
Copy link
Member Author

@alexswan10k This was bound to happen and it has happened rather soon 😉

Yeah, unfortunately it's a tradeoff between what you would expect from F# code and how React components behave internally. That's why I tried to use the type syntax so users couldn't nest the function component declaration within another function :)

If you don't need the display name and as you don't use memoization, you can just use the original ofFunction helper, like:

let myFuncComponent f =
   fun props -> ofFunction f props []

let rec complicatedRecFn threadedThing = function
    | A x->
        myFuncComponent(fun props ->
            div [] [someOtherFunctionComponent(threadedThing.value, x)]
            )
    | B children ->
        div [] [
            yield! children |> List.map (complicatedRecFn { threadedThing with SomethingElse = blah })
        ]
    | C -> ...

@alexswan10k
Copy link

Thanks for the tip @alfonsogarciacaro , this is useful to know actually.

@alfonsogarciacaro alfonsogarciacaro mentioned this pull request May 8, 2020
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.

5 participants