fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 67 forks source link

Simplified memo ? #118

Closed vbfox closed 5 years ago

vbfox commented 5 years ago

Little dilemma here... It's quite easy to extend memo support to get a much simplified usage, with this helper :

let simpleMemo (name: string) (render: 'props -> ReactElement) : 'props -> ReactElement =
    let memoType = memo name render
    fun props -> ofElementType memoType props []

Usage become as simple as :

// Definition
type HelloProps = { Name: string }
let hello = simpleMemo "HelloWorld" <| fun p ->
    span [] [str "Hello, "; str p.Name]

// Usage
let view model =
    hello { Name = "World" }

Up till there I would say :shipit: !

Until someone does that :

type HelloProps = { Name: string }
let view model =
    simpleMemo "HelloWorld" (fun p -> span [] [str "Hello, "; str p.Name]) { Name = "World" }

Or even that :

let view model =
    simpleMemo "HelloWorld" (fun () -> span [] [str "Hello, "; str model.Name]) ()

It has after all a very similar signature to all other helpers, except that this completely break all the memo optimizations...

Still I might find myself using it in a lot of my projects, so I don't know. What do you think ?

MangelMaxime commented 5 years ago

I am not sure to understand the benefit of simpleMemo ...

alfonsogarciacaro commented 5 years ago

I guess the benefit is you don't need to use ofElementType:

ofElementType helloWorldComponent { Name = name } []
// vs
hello { Name = "World" }

But if it makes it easier to write cases where you lose the optimization I'd say we should keep only the API that leads user into the pit of success (you said that :wink:). I'm a bit concerned than on one hand we're promoting the simplicity of Elmish and on the other users need to remember all the pitfalls of React. Savvy users can have a prelude with this kind of helpers for their projects :+1:

vbfox commented 5 years ago

You start from :

let hello name =
    span [] [str "Hello, "; str name]

let view model =
    hello model.Name

Now you saw my talk and want to add components & get better perf :

type HelloProps = { Name: string }
let helloComponent = memo "Hello" <| fun { Name = name } ->
    span [] [str "Hello, "; str name]

let hello p = ofComponentType helloComponent p []

let view model =
    hello { Name = model.Name }

But what i'm proposing is to remove the temporary helloComponent (That is always annoying to find a name for) :

type HelloProps = { Name: string }
let hello = simpleMemo "Hello" <| fun { Name = name } ->
    span [] [str "Hello, "; str name]

let view model =
    hello { Name = model.Name }
vbfox commented 5 years ago

One of the potential way to sidestep the problem would be to provide a more explicit name like memoBuilder, createMemoBuilder, ... something that echo the fact that the result should be saved & not used directly

vbfox commented 5 years ago

Ah one of the other things to consider is that while it break memo optimizations it's pretty harmless... it ends up acting like ofFunction so it's not much of a problem

Also the name memo (even "simple") is not something you might use accidentally so reading the function help might be something people would already do

MangelMaxime commented 5 years ago

Ah ok I see.

I guess if we educate correctly the person and show your minimal example as a doc comment of memoBuilder it should be fine.

Like that people can copy/paste the minimal example code and adapt it directly to their code.

alfonsogarciacaro commented 5 years ago

Hmm, memoBuilder with explanation in the comment would work. But I'd be in favor to keep only one API (even if I already released a version with memo) so users don't need to choose among many options and can just follow examples.

MangelMaxime commented 5 years ago

Ahah I guess that the comment explanation is ok as @alfonsogarciacaro and me answered the same thing at the same time :D

vbfox commented 5 years ago

Ok i'll do a PR changing the current signature

Move fast break things is one of the Facebook motto after all and they make React :)

vbfox commented 5 years ago

let memo = memoBuilder 😁

cmeeren commented 5 years ago

Why does memoBuilder need a string name but memo does not?

vbfox commented 5 years ago

memo would take the JS function name.

For anonymous functions F# doesn't support naming them.

For normal ones Fable use name mangling so the name wouldnot be good.

So in the end I preferred forcing a name to be provided.