Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inlining const React children #3673

Open
kerams opened this issue Dec 18, 2023 · 1 comment
Open

Inlining const React children #3673

kerams opened this issue Dec 18, 2023 · 1 comment

Comments

@kerams
Copy link
Contributor

kerams commented Dec 18, 2023

Description

Consider a simple div with 2 elements

let f () =
    div [ Class "gg" ] [
        div [] []
        span [] []
    ]

which compiles to

export function f() {
    const children_4 = [react.createElement("div", {}), react.createElement("span", {})];
    return react.createElement("div", {
        className: "gg",
    }, ...children_4);
}

This is fine, but we're left with a few unnecessary bytes (maybe there are even performance ramifications due to spreading?) and Terser doesn't handle similar cases either (screenshot of pretty print)

image

Could Fable recognize these single use consts and emit

export function f() {
    return react.createElement("div", {
        className: "gg",
    }, react.createElement("div", {}), react.createElement("span", {}));
}

or

export function f() {
    return react.createElement("div", {
        className: "gg",
    }, [react.createElement("div", {}), react.createElement("span", {})]);
}

(Not sure if there's a difference between the two, in performance or otherwise)

Related information

  • Fable version: 4.5.0
@MangelMaxime
Copy link
Member

While looking at this issue, I learned for the first time about [<ParamList>] attribute.

According to the changelog and the Fable.Core doc comment, it seems to only exist for Fable.React...

Changelog

## 3.0.0-nagareyama-alpha-003 - 2020-09-14

* Fix: Add names with $reflection suffix to root scope
* Fix deriving from imported JS classes
* Keep support for ParamList attribute until removed from Fable.React

Fable.Core

/// Used to spread the last argument. Mainly intended for `React.createElement` binding, not for general use.
[<AttributeUsage(AttributeTargets.Parameter)>]
type ParamListAttribute() =
    inherit Attribute()

type ParamSeqAttribute = ParamListAttribute

Looking thought the code and after an empiric test,
I am unsure what the effect of [<ParamList>] attributes is, looking through the code it seems to be of the same effect as when we use [<ParamArray>].

IHMO If that's confirmed it would be nice, to remove it in a future major version of Fable.

With that said, while testing what happen if we remove that attribute I discovered that the code should still be working without but it would not save the few extra bytes that you mentioned which is probably not what we want.

// With [<ParamList>]
export function f() {
    const children_4 = [react.createElement("div", {}), react.createElement("span", {})];
    return react.createElement("div", {}, ...children_4);
}

// Without [<ParamList>]
export function f() {
    const children_4 = [react.createElement("div", {}, []), react.createElement("span", {}, [])];
    return react.createElement("div", {}, children_4);
}

// With [<ParamArray>]
export function f() {
    const children_4 = [react.createElement("div", {}), react.createElement("span", {})];
    return react.createElement("div", {}, ...children_4);
}

Regarding the creation of the intermediate variable children_4, I am unsure how Fable decide to create it. What I mean by that is why Fable create a intermediate variable for children and not for the properties object {} in the exemple Without [<ParamList>].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants