fable-compiler / fable-react

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

Lack of Key in FunctionComponent.of #167

Closed Luiz-Monad closed 5 years ago

Luiz-Monad commented 5 years ago

I was having that pesky problem with React destroying all my DOM instead of patching it until I put key on my function components, including the root one used by Elmish-React.

I solved it this way:

let memoHighOrder fn equals name =
    let inline bindName ( n: string ) o = o?displayName <- n; o
    let fnC = fn |> bindName name
    let memoComp = 
        ReactElementType.memoWith equals fnC
    let memoFn children props = ReactElementType.create memoComp props children
    // Defeat Fable currying optimization, so things can be cached.
    fun ( key: string ) props ( children : ReactElement seq ) ->
        let bindKey props = props?key <- key; props
        let f = ( bindKey >> ( memoFn children ) ) |> bindName ( "Memo(" + name + ")" )
        f props 

I was thinking if it would be a good thing to expose a key, something like this perhaps?

    /// Creates a function React component that can use hooks to manage the component's life cycle,
    /// and is displayed in React dev tools (use `displayName` to customize the name).
    /// Uses React.memo if `memoizeWith` is specified (check `equalsButFunctions` and `memoEqualsButFunctions` helpers).
    static member KeyOf(render: 'Props->ReactElement,
                       ?displayName: string,
                       ?memoizeWith: 'Props -> 'Props -> bool)
                    : string -> FunctionComponent<'Props> =
#if FABLE_COMPILER
        match displayName with
        | Some name -> render?displayName <- name
        | None -> ()
#endif
        let elemType =
            match memoizeWith with
            | Some areEqual ->
                let memoElement = ReactElementType.memoWith areEqual render
#if FABLE_COMPILER
                match displayName with
                | Some name -> memoElement?displayName <- "Memo(" + name + ")"
                | None -> ()
#endif
                memoElement
            | None -> ReactElementType.ofFunction render
        fun key props ->
#if FABLE_COMPILER
            let bindKey props = props?key <- key; props
#else
            let bindKey = id
#endif
            ReactElementType.create elemType ( bindKey props ) []
alfonsogarciacaro commented 5 years ago

Yes, this is a problem we've had several times. You can pass the key in the props, but it's true that most people forget and it'd be nice to have it forced as a parameter. I'd personally avoid many FunctionComponent.Of variants because it can confuse users. Maybe we could force 'Props to implement key through an interface (although that would be a breaking change) or we could add an optional argument for a projection to get the key from the props, like:


static member Of(render: 'Props->ReactElement,
                       ?displayName: string,
                       ?memoizeWith: 'Props -> 'Props -> bool,
                       ?withKey: 'Props -> string) =
    ...
    fun props ->
#if FABLE_COMPILER
            let bindKey props =
                match withKey with
                | Some f -> props?key <- f props; props
                | None -> props
#else
            let bindKey = id
#endif
            ReactElementType.create elemType ( bindKey props ) []
MangelMaxime commented 5 years ago

Maybe we could force 'Props to implement key through an interface (although that would be a breaking change)

I don't think a Key is always needed. It depends on the context of the component.

we could add an optional argument for a projection to get the key from the props

I personally prefer this option :)

Luiz-Monad commented 5 years ago

Yes, the optional argument would remind you about the key if you needed it.