fable-compiler / fable-react

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

Cannot pattern match Style #174

Closed halcwb closed 4 years ago

halcwb commented 4 years ago

Why can this code not be compiled to js?

let combineStyles st1 st2 =
    match st1, st2 with
    | Style p1, Style p2 -> p2 |> List.append p1 |> Style 
    | _ -> [] |> Style

I get:

Module Error (from ./node_modules/fable-loader/index.js): ... error FSHARP: The pattern discriminator 'Style' is not defined. (code 39)

Is this because of the ICSSProp vs CSSProp issue?

MangelMaxime commented 4 years ago

Because when compiling against Fable Style isn't a DU case.

Source

We need to do that, in order to convert the Style arguments into a JavaScript object as expected by React.

halcwb commented 4 years ago

Thanks, so there isn't a way for what I want to achieve?

MangelMaxime commented 4 years ago

Well, there are ways to achieve what you want but it relies on dynamic typing.

open Fable.Core
open Fable.Core.JsInterop
open Fable.React
open Fable.React.Props

[<Emit("Object.assign({}, $0, $1)")>]
let combineObjects (obj1 : obj) (obj2 : obj) = jsNative

let (|IsStyle|_|) (value : HTMLAttr) =
    if JS.Array.isArray(box value) then
        let _, style = unbox value
        Some (unbox<CSSProp list> style)
    else
        None

let combineStyles st1 st2 =
    match st1, st2 with
    | IsStyle p1, IsStyle p2 ->
        // p1 and p2 have already been converted to an JObject by Style helpers
        // some we need to combine both objects
        !! ("style", unbox combineObjects p1 p2)
    | _ -> 
        Style [ ]

let shouldReturnEmptyStyle = 
    combineStyles 
        (Disabled true) 
        (Style [ Height 20 ]) 

let shouldAppendBothStyles = 
    combineStyles 
        (Style [ Width 20 ]) 
        (Style [ Height 20 ]) 

JS.console.log(shouldReturnEmptyStyle)
JS.console.log(shouldAppendBothStyles)

Note that this code will only work against Fable runtime. If you are running it against SSR (.Net runtime) Style is indeed, in this case, a DUs Case. If you need to support both runtimes you can use compiler directives.

I am closing this issue, as the current behaviour is "normal" because of what React is expecting us to feed him with.

alfonsogarciacaro commented 4 years ago

Yeah, this is a bit unfortunate. I didn't think of pattern matching when creating the ugly hack to convert the Style list argument into a JS object. Maybe we could fix it by checking here if the value is a list and then converting it into a JS object too (we did something similar in the past). The only problem is that would only work with latest versions of the compiler 🤔

MangelMaxime commented 4 years ago

Maybe we could fix it by checking here if the value is a list and then converting it into a JS object too

I think the case described by @halcwb is very rare and I am afraid of breaking the code of someone by introducing this new behaviour. What if you really want to pass a list and not an object?

In general, if you are working with a different source for the style, you should just work at the list level and concatenate them at the very end before passing them to Style.

let myView = 
    let style1 = // Some code returning a list
    let style2 = // Some code returning a list

    let myFinalStyle = style1 @ style2 |> Style

    div [ myFinalStyle ]
        [ ... ]

Especially because combineStyles seems to discard the style if there are one style and another property but perhaps this is just a minimal reproduction code.

halcwb commented 4 years ago

@MangelMaxime: Agree, thanks for looking into this.