fable-compiler / fable-react

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

Inconsistent types across a compiler directive cause build warnings when using Fable.React.Helpers #103

Closed talbottmike closed 5 years ago

talbottmike commented 5 years ago

There is a discriminated union for HTMLAttr which has an #if !FABLE_COMPILER at the end and the type has the interface IHTMLProp.

    type HTMLAttr =
        | DefaultChecked of bool
        | DefaultValue of string
        ...
#if !FABLE_COMPILER
        | Style of CSSProp list
        | Data of string * obj
#endif
        interface IHTMLProp

Then, later in the same module, there is an inline function called "Data" inside of an #if FABLE_COMPILER.

#if FABLE_COMPILER
    let inline Style (css: CSSProp seq): HTMLAttr =
        !!("style", keyValueList CaseRules.LowerFirst css)

    let inline Data (key: string, value: obj): IHTMLProp =
        !!("data-" + key, value)
#endif

Fulma uses the Data function in the Divider module. When building the Fulma docs, the compiler emits a warning.

WARNING in ../src/Fulma.Extensions/Divider.fs C:/Workspaces/GitHub/talbottmike/Fulma/src/Fulma.Extensions/Divider.fs(51,39): (51,74) warning FSHARP: This upcast is unnecessary - the types are identical @ ./src/FulmaExtensions/Divider.fs 5:0-75 @ ./src/FulmaExtensions/Router.fs @ ./src/App.fs @ ./docs.fsproj @ multi ../node_modules/webpack-dev-server/client?http://localhost:8080 webpack/hot/dev-server babel-polyfill ./docs.fsproj

This warning occurs both when building the docs and when using the Fulma divider in downstream projects.

In Fulma the case is required by the editor, but the compiler emits a warning when building. I submitted a pull request for Fulma but @MangelMaxime suggested this be addressed in fable-react. The Fulma pull request is here.

https://github.com/MangelMaxime/Fulma/pull/160#issue-217831865

The proposed change would make the types consistent in both the editor and when compiling. Thanks

alfonsogarciacaro commented 5 years ago

Good catch @talbottmike ⚾️ Thanks a lot for the detailed explanation!