[WIP] Use case names instead of literals for class names

[alfonsogarciacaro]

First draft for #173. With this change (only in Column.fs file) I'm already saving 10KB in the minified bundle. I assume it will be more or less the same for other projects because the change is internal to Fulma.

[alfonsogarciacaro]

@MangelMaxime What do you think of this? Should I keep going and update other modules?

[MangelMaxime]

Yes, please the changes seems reasonable and the impact significative. :)

[alfonsogarciacaro]

This is using ofHidden screen (and two lines above ofInvisible screen). Is it an error? I've changed them to ofHiddenOnly and ofInvisibleOnly respectively.

[MangelMaxime]

Yes, I guess it was an error I will need to check when reviewing the whole PR.

[alfonsogarciacaro]

@MangelMaxime I've updated the Button and Common modules too. There's some information lost in the intermediary Options types, as I'm generating the classes directly. But I couldn't find anywhere where this intermediary information is used, so I hope it's fine.

We can update the other modules for consistency (it'd be great if somebody could help with that), but these should be the ones with higher impact in the bundle size.

Note: if you plan to release a beta with these changes, I'm thinking of changing a bit the Fable.React (and Fable.Import.JS) namespaces so it could be a good opportunity to update the dependencies. I'll raise an issue about this shortly.

[MangelMaxime]

@alfonsogarciacaro I will convert the others modules to be consistant.

Should we convert all of them in a dummy way or is there some modules where using the new code would increase the bundle size ?

[alfonsogarciacaro]

I wanted to quickly update the Breadcrumb module so you could use it as reference, but as usual I got carried away and started to simplify the code to parse the options by removing the internal Options type and leaving only GenericOptions (in Common.fs) with many helpers. This reduces the code a lot so the maintenance should be easier (and bundle size smaller) but the inconvenient is now we're indeed forced to update all the modules.

Please have a look at this commit and let me know what you think, I can revert it if necessary.

[MangelMaxime]

So the idea is to build "directly" the attributes list.

Well, the code seems simplify and easier to read so I don't see any reason to not accept the proposition :). Also, because this improve the bundle size its another pros :)

About removed empty classes yes, this seems a good idea. I never really took the time to do it but @forki asked if we could fix it.

This can make sense for people using SSR and even to give people a nicer feeling about the "generated code".

[alfonsogarciacaro]

Hmm, now that you mention it. SSR is going to get tricky, because the CompiledName attribute doesn't work for union cases in .NET :/ So we may need to keep the old code for the .NET version. Or there may be another solution, let me think about this.

[alfonsogarciacaro]

Ok this seems to work, so we can still get the CompiledName in .NET using some reflection.

[forki]

I'm very pleased to see that you are thinking about SSR. Fulma and SSR is awesome Am Mi., 23. Jan. 2019, 17:23 hat Alfonso Garcia-Caro < [email protected]> geschrieben:

…

Ok this seems to work <https://amazingant.com/blog/2015/03/13/custom-attributes-on-fsharp-union-cases/>, so we can still get the CompiledName in .NET using some reflection. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#177 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNGig7DbaSsKz2zCA380snGmiL5vIks5vGIyQgaJpZM4ZLVe3> .

[alfonsogarciacaro]

Looking good!

[MangelMaxime]

@alfonsogarciacaro Ok, so I can keep converting everything in a dummy way and SSR will be supported?

[alfonsogarciacaro]

To support SSR I need to refactor this function and create one with conditional compilation that executes special code in .NET (like Fable.React functions do for SSR).

But we can do that later after confirming everything is working for Fable. When using Fulma on the server we don't care about the code size so there's no urge to update the library in that case.

[MangelMaxime]

Ok thank you for confirming. I will go back to converting the library tomorrow :)

[MangelMaxime]

So I convert almost all the project.

The Fulma.Extensions.Wikiki.Calendar and Fulma.Elmish.DatePicker have not been yet converted. I need to convert the code from the CSS definition from scracth (we are supporting version 0.1.7 while the latest is 5.0.x).

So with Fulma.Extensions.Wikiki.Calendar and Fulma.Elmish.DatePicker removed from the current PR, we get the following result on the Fulma docs site:

• Old version: 490 KiB
• New version: 401 KiB

So we have an 89KiB reduction 😱😱😱 for the bundle generated in production mode. In theory, the final gain of size should be a bit smaller because as I said I am missing 2 libraries ATM.

I will try to have a more detailed size report in the future to see the exact size of Fulma so we can have an idea of the % of reduction.

I tested the bundle size against the Fulma docs site because it's using all the modules of Fulma ecosystem.

[MangelMaxime]

So I extracted Fulma chunks in a separated file:

    optimization: {
        // Split the code coming from npm packages into a different file.
        // 3rd party dependencies change less often, let the browser cache them.
        splitChunks: {
            cacheGroups: {
                commons: {
                    test: /node_modules/,
                    name: "vendors",
                    chunks: "all"
                },
                fulma: {
                    test: /src\/Fulma/,
                    name: "Fulma",
                    chunks: "all"
                }
            }
        },
    },

• Old version: 400 KiB
• New version: 312 KiB

So if my calculus is correct, the new version represents 78% of the old version and so we have currently a reduction of 22%.

[alfonsogarciacaro]

I'm assuming the size gain will be bigger in most other projects. This is because the Fulma docs already includes ALL the class names (for the examples), while in a common project you'll be only using a handful of them (and the purpose of this PR is not to include the unused class names in the bundle size). We should check with the fulma-demo of the SAFE template.

[MangelMaxime]

@alfonsogarciacaro Yes, I agree and will test on Fulam-demo when ready.

I was just testing the worth scenario here which already show a good reduction :)

[MangelMaxime]

I have added .Net support for SSR.

Thank you @alfonsogarciacaro for helping on this PR.

I will merge it as all the package has been converted except the calendar. This one I am planning to replace it with another the creator of the package removed the initial documentation and focused on providing a JS version of the components. And I don't understand all the things implemented in sass. I will create a replacement library for Fulma.

At the same time before publishing, I am working on the new doc site and split the project between several repos for easier maintenance.

[alfonsogarciacaro]

Great @MangelMaxime! 💪 How did you add support for .NET? Please note Fable.Core.Reflection.getCaseName doesn't work in .NET, we have to write a polyfill for that.

[MangelMaxime]

I made a polyfill using conditional compilation.

I tested the .Net we a small reproduction in FSI and it was working from my test.

FSI test code

module Test

open Microsoft.FSharp.Reflection
open System

type Option =
    | [<CompiledName("is-active")>] IsActive
    | CustomClass of string

let getCaseName (case : 'T) =
    // Get UnionCaseInfo value from the F# reflection tools
    let (caseInfo, _args) = Microsoft.FSharp.Reflection.FSharpValue.GetUnionFields(case, typeof<'T>)
    // Pull all attributes
    let attributes = caseInfo.GetCustomAttributes()

    let haveError =
        attributes
        |> Seq.ofArray
        // Filter for the FatalError values
        |> Seq.filter (fun x -> x :? CompiledNameAttribute)
        // Cast each value
        |> Seq.map (fun x -> x :?> CompiledNameAttribute)
        // Seq.take breaks if there aren't enough, so use Seq.truncate instead
        |> Seq.truncate 1
        |> List.ofSeq

    if haveError.IsEmpty then String.Empty
    else haveError.Head.CompiledName

[alfonsogarciacaro]

Good one! May I suggest a slightly shortest version also with a cache so we don't need to use reflection every single time?

// For this use case, using the cases as object references for keys should be enough
let cache = ConcurrentDictionary<obj, string>()

let getCaseName (case : 'T) =
    cache.GetOrAdd(case, fun _ ->
        let (caseInfo, _args) = FSharpValue.GetUnionFields(case, case.GetType())
        caseInfo.GetCustomAttributes()
        |> Array.tryPick (function
            | :? CompiledNameAttribute as att -> Some att.CompiledName
            | _ -> None)
        |> Option.defaultValue caseInfo.Name)

Maybe the cache is not needed but Reflection is usually slow so this could improve performance.

Also, there's a Fable.Core.Reflection call here that it hasn't been replaced with the cross-platform version yet.

[MangelMaxime]

May I suggest a slightly shortest version also with a cache

How dare you suggest something 😱

Of course, you can and that's a good idea indeed. I will adapt the code.

For the call that hasn't been replaced, it's only for the documentation so it's fine if we use only Fable version. And also, I didn't replace it because the code doesn't generate the expected behavior (wrong class I guess) here but I didn't check more than that as I am reworking the documentation system.