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

transformJsxProps errors on large strings #3839

Open
joprice opened this issue Jun 11, 2024 · 3 comments
Open

transformJsxProps errors on large strings #3839

joprice opened this issue Jun 11, 2024 · 3 comments

Comments

@joprice
Copy link
Contributor

joprice commented Jun 11, 2024

Description

There seem to be inputs of a certain size that trigger a transformation from a string like "a" to the expression let x = "a" in x. Since transformJsxProps is checking for a specific pattern of a tuple expression for the prop's key and value, the pattern match fails and the error Cannot detect JSX prop key at compile time is triggered. This can be worked around by moving the literal into a local variable, but some frontend patterns like long tailwind classes, hardcoded urls, and svg values are often longer than 100 characters, making it an inconvenient limitation.

Changing the pattern match to destructure the Let binding works, but I'm unsure there aren't cases where this would produce an incorrect result:

 | Some(props, children),
                (Fable.Value(Fable.NewTuple([ StringConst key; value ], _), _)
              | Fable.Let(_, value, Fable.Value(Fable.NewTuple([ StringConst key ; _], _), _))) ->

Repro code

main...joprice:Fable:jsxError

<JSX.Component>]
let classTest () =
    Html.div [
      Attr.className
        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
    ]

Expected and actual results

Literals of any size should not cause compilation errors or require workarounds to use in JSX expressions.

Related information

  • Fable version: 4.18.0
  • Operating system: OSX
@joprice
Copy link
Contributor Author

joprice commented Jun 11, 2024

I found the commit that limits inlining of string constants to those less than a length of 100. Which makes sense, since the above code works in versions before 4.0.4 4.0.3...4.0.4#diff-ea3bc6b45d9295f9c8bf3f1acdaf7b8d94fb1bc7eb60fcedc3027710b4abf2e2R186

@MangelMaxime
Copy link
Member

I am unsure what canInlineArg does, I mean for which code it does inline the string. I think the only case I saw a literal string being inlined was when using [<Literal>] on it which doesn't seems to be controlled by the canInlineArg function.

Why do we have an arbitrary limitation on string that can be inlined or not. Is this to minimise the impact on bundle size or is there another reason?

@joprice
Copy link
Contributor Author

joprice commented Jun 12, 2024

If I increase the limit from 100 to 200, it does work as expected, so I think it's a case like the following

With an inline definition like

static member inline src (value: string): JSX.Prop = "src", value

and an argument to the inline function that is considered small enough:

Attr.src "a"
// becomes
("src", "a")

When the argument to the inline function is considered too large:

Attr.src "a......"
// becomes
let x = "a......"
("src", x)

However, it seems that this is only useful when the input is a let binding with more than one reference, not when it's a literal.

let veryLongStringThatCouldBeInlined = "a......."
("src", veryLongStringThatCouldBeInlined)
let veryLongStringThatShouldntBeInlined = "a......."
("src", veryLongString)
("src", veryLongString)

But maybe there's a case for it I'm not aware of.

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