-
Notifications
You must be signed in to change notification settings - Fork 311
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
Rolling out nullness annotations #3887
Comments
Not only the binding needs to be updated to support nullness supports but we will also need to update the FCS fork that Fable use so we can get access to the language feature. Are the configuration you mentioned to be set on the binding project or do the user project also needs to set it? What I mean is what happens if we decide to use nullness for the binding but the user project doesn't active the language feature on its project? Will he still have the error generated? |
True, but that should be easy enough. Also, you'll get warnings in the IDE regardless.
Ah, I just realized Fable packages include source files, this makes things much more complicated. |
I am tracking the nullness supports in Glutinum project here. It ask questions regarding how things should be represented. It could also be a good time for checking how Glutinum handle Dom APIs. I always had the vision to be able to generate such APIs because they are kind of big and being able to generate them means we should be able to applied optimisation to them over time. The original version of Fable.Browser, was actually generated and over time we stopped doing that because the project was splitted in different package. It was to simply the maintenance I believe but over time we discovered that it was not so much the case. |
So if I read correctly, if we decide to use |
If you switch an existing option annotation to |
Ah yes right, this is one of the reason we use F# after all 😅 (sometimes you forget some of the simplest things). |
Since we have to distribute source files, I think the only option is to use conditional compilation for annotations. Something like #if NET9_0_OR_GREATER
abstract getElementById: elementId: string -> HTMLElement | null
#else
abstract getElementById: elementId: string -> HTMLElement
#endif but then we'll also need to add |
If we need to duplicate the API surface to support nullness then I will probably vote against it, we need to either commit to it or keep using Option for the time being. Duplicating the API surface means depending on your configuration you have a different API which his confusing I wonder why for this new feature people need to use target
After thinking, I think I understand why you want to do that. This is to have support for compiler directives to branch out the API with or without nullness. If I remember how TFM works, in order to use net9.0 in your project you need to use .NET 9.0 SDK. And this is problematic because it means forcing people to upgrade to .NET 9.0 which is not a LTS version. I think people tend to avoid upgrading in general and favor LTS. That NET 9.0 upgrade is not so much a problem for Fable because we don't really respect/use that information. But it is a problem for Server code I think. I suppose the fact that F# distribute source files, is what prevents this part of the RFC to works:
Depending what can or cannot be done, I think it is also possible to use a custom type to avoid duplicating the code: type Nullable<'T> =
#if NET9_0_OR_GREATER
'T | null
#else
'T option
#endif It still provides 2 different API depending on the configuration but at least doesn't duplicate code in the binding/library code. |
I decided to move this issue into Fable repository because there is more to decide that the aesthetic of the API. @ncave As you probably know more about FCS + Fable internals, what is your vision on how nullness could be added to Fable? |
To avoid the need for targeting |
You need .NET 9 to use nullness in the first place, so I don't see why requiring
Technically speaking, I'd say it's the same API surface with extra metadata. In any case, that's just how C# and now F# work. With different values of
Precisely.
This will give you truly different API surfaces. In Fable JS there isn't going to be any difference in the transpiled output as far as I am aware (options only exist at compile time), but with projects compiled with .NET, you get incompatible IL and run-time differences. Most importantly, you will also need different method bodies. |
Perhaps, but it still means that as a maintainer you need to double the amount of code you maintains. And personally, I don't think I would do it, having bindings with thousands of lines is already not fun to maintain 😅 (so if I need to double the amount ...) Especially, because using
I see, my vision was primarily focused on Fable because the original discussion was about bindings. And from Fable POV, Then perhaps the way to mimic what you are describing is: type Nullable<'T> =
#if NET9_0_OR_GREATER
'T | null
#else
'T
#endif For me, it is also difficult to take a decision on such things without being able to play with the code. Because, there are a lot of things that I don't see without having a compiler result / code output to check. |
As an independent observer, I would not recommend replacing existing options with I would consider adding the Nullable annotation around values that have not been optional until now, but were practically shown to be null at runtime in various cases. |
@MangelMaxime Currently Fable transpiles F# options to On a side note, the best explanation of nullable types I've ever read was, oddly enough, in Dart documentation. |
Thanks both of you for your input. Personally, my current position is:
|
I am starting to look at how to implement I think we could map We could make some Fable magic to map This would gives us the ability to support If we map |
For comparison, Scalajs did a similar solution https://www.scala-js.org/api/scalajs-library/latest/scala/scalajs/js/index.html#UndefOr[+A]=A@scala.annotation.unchecked.uncheckedVariance|Unit. Scala 3 has union types, but Scalajs supports scala 2 as well, so has it's own union type that uses implicits, similar to |
We need a way to model certain JS APIs (e.g. export type Nullable<T> = T | null | undefined;
export type Option<T> = T | Some<T> | undefined; IMO it makes most sense to use the new nullable types as |
Here are some examples code with their compilation equivalent trying to explains my point of view: Mapping nullness to
|
You're all talking about TS type annotations, right? When targetting JS, I'd expect |
We are talking about which behavior it should have when targeting JS/TS. The distinction can be/is needed for interop with JavaScript libraries, in JavaScript
|
OK, but if you're saying that we need to be able to tell the difference in F# too and an API accepts or can return |
Yes, I know that to do we need But IHMO we need to plan for when it will be available, anonymous DUs is planned by the F# team but they are waiting for C# to implement it first for compatibility. So "we know" it is coming. However, if we originally map nullness to |
It's coming who knows when. Regardless, we can close the issue then? |
I know ...
I don't think we support nullness yet in Fable at least no work was done in that direction beside updating the FCS. We need to check what actually happens when nullness is used with Fable currently. |
The waiting is mainly for possible changes in the runtime representation, which of course are not relevant for Fable. For the low level scenarios that need to see a difference (i.e., should not be the default because of ergonomy), is there a reason why something like: Reason is, besides not having to wait for anonymous unions, this would also free Fable from .NET-imposed limitations on the F#9 |
I don't know well I didn't find information about "limitations [...] mainly around generic code" but I found this section: Which seems to defeat my idea of using Because of that it seems like we need to consider nullness supports as a F# feature, and not so much as a JS/TS equivalent to I am also not sure what you mean by:
Here you are using a real DUs right? I suppose we could indeed make it work, I am not sure how much magic from Fable we would need to do it. |
@MangelMaxime Do you have a binding example of why can't we just transpile the F# nullable types ( export type Nullable<T> = T | null | undefined; |
I don't have one under the hand not, and the more the discussion goes and the more compiling to the existing seems to be ok for me. My initial reason was to make a distinction in the future and have almost a 1 to 1 match with TypeScript syntax which is always good for bindings but this is not possible. |
I think the next step now is to give it a try to compile to: export type Nullable<T> = T | null | undefined; add some tests to check how it plays and adapt if needed. |
A real DU, yes. I think you got it correctly: |
Then there is the question of how to properly represent the Nullable Reference Types in Fable.AST. Once we come up with some tests for |
.NET 9 is around the corner, and with it support for consuming and emitting nullness metadata in F# projects. JS APIs will often return
null
/undefined
, and while Fable bindings often capture this information by using options, sometimes we give no indication thatnull
s are a possibility. I think F# 9 will be really useful here, so I'd like to start a discussion about the adoption plan for these annotations.From the technical point of view, binding projects need to be compiled using .NET 9 SDK,
<LangVersion>preview</LangVersion>
(for now) and with the--checknulls+
flag (also for now - I think this will be aligned with C#'s<Nullable>enable</Nullable>
). TFMs don't matter, sonetstandard2.0
assemblies will contain nullness metadata too.Finally, every API will need to be manually reviewed and annotated as required. In the case of existing bindings that use options with reference types, there's the question of whether we want to preserve options or replace them with
null
annotations, just so that a single, unified approach is used.The text was updated successfully, but these errors were encountered: