-
Notifications
You must be signed in to change notification settings - Fork 301
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
Latest Fable fails to compile Fable.React #3658
Comments
I agree that generating the check on site is better as this will reduce the bundle size and for this kind of checks I think this the good path to take. |
Are people no longer using Fable.React or just not upgrading? I am a little surprised no one else has complained about this critical issue. |
@kerams I'm not at all familiar with Fable.React bindings, so please excuse my ignorance, but I wonder if there is a better way to express arbitrary properties, instead of using an erased union case (which kind of defeats the purpose of using unions to model properties in the first place). I understand that's not helping with the existing version of Fable.React bindings, it's just that using erased union cases is not even documented in Fable docs, it looks like a very custom workaround, so I wonder if that's the best we can do. e.g. perhaps something like this can work instead (i.e. lifting arbitrary props one level up): open Fable.Core
type HTMLProp =
| Href of string
type HTMLAttr = U2<HTMLProp, string * obj>
let test (attr: HTMLAttr) =
match attr with
| U2.Case1 (Href s) -> s
| U2.Case2 (s, o) -> s
let x: HTMLAttr = U2.Case1 (Href "https://")
let y: HTMLAttr = U2.Case2 ("something", 10)
assert (test x + test y = "https://something") I'm not saying this is a good solution or that it's ergonomically the same, it's just a question. |
While that would work, it's a massive breaking change since you'd suddenly need |
If it is not documented this probably a mistake on my end, as I forgot to add it when doing the documentation rework.
We could probably workaround the I am sorry that this bug has not been fixed yet, I always had it in the back of my head but never had the time or courage to dig into it. I didn't touch this portion of Fable yet. 😅 I will make sure to give it a try this week and as this is not only breaking Fable.React but also impacting the bundle size. |
Thank you, Maxime. |
Are you perhaps running .NET 9.0? Correct me if I'm wrong, but I don't think dotnet/fsharp #16341 is in .NET 8.0, since it was only merged on Dec 7, 2023, so that may be the reason why you don't see it (yet). Anyway, IMO we have several ways we could fix this for the next Fable release:
IMO they all have pros and cons, e.g. being more explicit about intent vs backwards compatibility with existing bindings. Here is approximately where a change can be implemented to skip some or all of the compiler-generated members that match one of the criteria above. |
I am indeed on 9 (langversion and SDK-wise), but don't see why it matters since Fable uses its bundled FCS, not fsc in the SDK. The former contains changes from the PR you mentioned, so you can also replicate the issue in Fable repl. |
@kerams That is correct, our current FCS snapshot is as of Dec 8, 2023, so it includes it. I just meant it's not technically part of .NET 8.0. Anyway, @kerams @MangelMaxime please chime on the options above, and I can make that change. |
@ncave The problem is that in Fable 4.8.0 we updated the version of FCS. But that version of FCS is having new F# features added to it which are for F# 7 (I believe). More specifically dotnet/fsharp#16341 which exposed Because of that, Fable output changed between Fable 4.7 and Fable 4.8. I don't know why but I don't have the same generated code between the REPL and Fable running locally for this case... Could it be that the new feature is only effective in Fable too if user is using .NET 9? @kerams Can you try forcing .NET 8 to your project using type HTMLAttr =
| Href of string
| Custom Fable 4.8+ generated code (from the REPL) import { Union } from "fable-library-js/Types.js";
import { union_type, string_type } from "fable-library-js/Reflection.js";
export class HTMLAttr extends Union {
constructor(tag, fields) {
super();
this.tag = tag;
this.fields = fields;
}
cases() {
return ["Href", "Custom"];
}
}
export function HTMLAttr_$reflection() {
return union_type("Test.HTMLAttr", [], HTMLAttr, () => [[["Item", string_type]], []]);
}
export function HTMLAttr__get_IsHref(this$, unitArg) {
if (this$.tag === 0) {
return true;
}
else {
return false;
}
}
export function HTMLAttr__get_IsCustom(this$, unitArg) {
if (this$.tag === 1) {
return true;
}
else {
return false;
}
} Fable 4.7 (and below) generated code: import { Union } from "fable-library-js/Types.js";
import { union_type, string_type } from "fable-library-js/Reflection.js";
export class HTMLAttr extends Union {
constructor(tag, fields) {
super();
this.tag = tag;
this.fields = fields;
}
cases() {
return ["Href", "Custom"];
}
}
export function HTMLAttr_$reflection() {
return union_type("Test.HTMLAttr", [], HTMLAttr, () => [[["Item", string_type]], []]);
} See that But instead of generated them as standalone functions, I am in favour of generating them in place (inline). This will limit the impact on bundle size even if that should be removed by any decent bundler nowadays. But more importantly it will keep supporting: type HTMLAttr =
| Href of string
| [<Erase>] Custom. of string * string which is the historical way for doing React bindings and a feature of Fable do generate POJO from DUs. open Fable.Core
open Fable.Core.JsInterop
type HTMLAttr =
| Href of string
| [<Erase>] Custom of string * string
let o =
keyValueList CaseRules.LowerFirst [
Href "url"
Custom ("data", "value")
] generates: import { declare, Union } from "fable-library/Types.js";
import { union_type, string_type } from "fable-library/Reflection.js";
export const HTMLAttr = declare(function Test_HTMLAttr(tag, name, ...fields) {
this.tag = tag | 0;
this.name = name;
this.fields = fields;
}, Union);
export function HTMLAttr$reflection() {
return union_type("Test.HTMLAttr", [], HTMLAttr, () => [["Href", [["Item", string_type]]], ["Custom", [["Item1", string_type], ["Item2", string_type]]]]);
}
export const o = {
href: "url",
data: "value"
}; |
I am not sure if this is a good idea of not to do it this way as we divide a bit from F# default code. |
We'll have to suppress these member declarations anyway, in order to inline them, so that sounds like an additional step to fix the call sites. That may not be necessary if the bundler will not include them anyway, unless used. Anyway, sounds like this use case is very specific for JS/TS bindings, so whatever we decide to do needs to be well documented. |
Getting the same outcome regardless of global.json, TFM and langversion. |
Ok, I am not sure why I don't have them personally. The more I dig, and the more I think the problem is perhaps due to the error message instead. Fable 2 output: type HTMLAttr =
| [<Fable.Core.EraseAttribute>] Href of string
| Custom of string
let a = Href "href" = Custom "custom" import { declare, Union } from "fable-library/Types.js";
import { union_type, string_type } from "fable-library/Reflection.js";
import { equals } from "fable-library/Util.js";
export const HTMLAttr = declare(function Test_HTMLAttr(tag, name, ...fields) {
this.tag = tag | 0;
this.name = name;
this.fields = fields;
}, Union);
export function HTMLAttr$reflection() {
return union_type("Test.HTMLAttr", [], HTMLAttr, () => [["Href", [["Item", string_type]]], ["Custom", [["Item", string_type]]]]);
}
export const a = equals(["href"], new HTMLAttr(1, "Custom", "custom")); It is true that this code However, doing a comparaison on Href "href" = unbox ([| "href" |])
// generates
equals(["href"], ["href"]); but is it a real problem? Yes, think this has always been like that. I mean this is what Fable 2 generates and I don't anyone complained about it. Something even more strange is that I cannot reproduce the Fable/tests/Js/Main/JsInteropTests.fs Lines 137 to 139 in e2285d6
The code linked above is enough to trigger the error in the REPL but Fable CI / Tests is not complaining about it. Which is another hint in the direction of having an issue with the error message instead of the generated code instead. Any idea, why @kerams machine and the REPL fails while the CI and my machine succeed? And I suppose @ncave machine also works because they send PR and would have seen the test failing locally I think. |
I don't think so. All I can think of, without digging further, is that there are probably some additional checks for .NET version in FCS that prevent the new properties from being generated. For me, |
I tried with .NET 9 installed and can't reproduce either with a minimal reproduction. @kerams Could you please confirm that my reproduction project fails for you too? https://github.com/MangelMaxime/fable-react-reproduction-3658 I didn't use Fable.React in it because I can reproduce in the REPL without it. But if the reproduction fails, please feel free to send PR with Fable.React or tells me to include it. Not being able to reproduce the bug is not help us here 😅 Edit: I forced a specific version .NET 9 to try minimise the source of difference. |
@MangelMaxime I can now reproduce on .NET 8.0 with Fable 4.19, if I add |
Yes, works without langversion, since case testers are not exposed without Add |
Great news 🎉 Never been so happy to see some red ahah. But now, we are back to understand what code generation is causing this error. |
Coming back to @ncave proposition
If possible, I would avoid adding a new attribute. This will needs all binding that use this syntax to update. Also,
I believe this proposition is a good compromise between restoring support for DUs used for interop to create POJO with an If we do that, we should probably add a warning / error is user is trying to call any of the compiler generated members. This is because the IDE will not report this issue which is Fable specific.
I think going for the option before, is best for consistency? What I mean by that is that like that none of the compiled generated member are available at all. So user don't need to question himself why some are available and some not. |
You can use this to decide whether to generate a union member or not - |
@kerams For some reason Not sure if that's expected, perhaps it has been fixed after we took the FCS snapshot, but it sounds like a bug. Perhaps it's just missing the member _.IsUnionCaseTester =
checkIsResolved()
match d with
| P p -> p.IsUnionCaseTester
| M m -> m.IsUnionCaseTester
| V v -> v.IsPropertyGetterMethod && v.LogicalName.StartsWith("get_Is") // <-- fix
| E _ | C _ -> invalidOp "the value or member is not a property" So for now I'll use |
@kerams Opened an issue in F# repo, see dotnet/fsharp#17301 |
Works great, many thanks, sirs, and sorry for being impatient. |
No worries, thanks for confirming that all works now. You can enjoy features/fixes from more than 10 releases since this bug 🎉 |
Description
.\src\Client\fable_modules\Fable.React.9.3.0\Fable.React.Props.fs(43,5): (43,12) error FABLE: Cannot test erased union cases
This is because the bundled FCS contains dotnet/fsharp#16341, and Fable tries to create case tester functions for every case.
for
Adding
[<Fable.Core.Erase>]
toHref
orCustom
introduces a compiler error.I suggest Fable is adjusted to recognize the newly exposed case testers and to emit tag comparisons at call sites instead of creating properties. That way compilation will only fail when trying to use a case tester on an erased case.
Repro code
https://fable.io/repl/#?code=C4TwDgpgBAEgKgWQDIEFjAE5QLwCgoFQA+UAFAFRQDaAPAGICGARgDYQB0AwgPYYcCiGBgGcIAPgC6UcgEoYfAGZRuS4ZgCWAOwDm+QiU4BXNdwC2uXG2BQAHjihGTp9gElhj4GaA&html=Q&css=Q
Related information
The text was updated successfully, but these errors were encountered: