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

[Feature Request] Add additional compiler directives #206

Closed
HLWeil opened this issue Sep 25, 2024 · 16 comments
Closed

[Feature Request] Add additional compiler directives #206

HLWeil opened this issue Sep 25, 2024 · 16 comments

Comments

@HLWeil
Copy link

HLWeil commented Sep 25, 2024

We stumbled across a very intricate, general problem with Fable. The base problem is, that python fable transpilation fails, if emitted javascript code is referenced somewhere down the line.

In order to fix this, we use conditional dependencies in https://github.com/nfdi4plants/ARCtrl/blob/main/src/Json/ARCtrl.Json.fsproj:

  <ItemGroup>
    <PackageReference Include="Thoth.Json.Core" Version="0.2.1" />
    <PackageReference Include="Thoth.Json.Newtonsoft" Version="0.1.0" />
    <PackageReference Condition="'$(FABLE_COMPILER_PYTHON)' == 'true'" Include="Thoth.Json.Python" Version="0.2.0" />
    <PackageReference Condition="'$(FABLE_COMPILER_JAVASCRIPT)' == 'true'" Include="Thoth.Json.JavaScript" Version="0.1.0" />
  </ItemGroup>

This works in the repository, when calling the published package on nuget and also the javascript package in npm and the python package in pypi work.
However, a downstream library depending on the nuget package cannot be transpiled to js or python, as the dependencies to Thoth.Json.Python and Thoth.Json.JavaScript are not included in the nuget package. Fable therefore does not include these dependencies.

I looked at this from many different angles, and the only working solution I found is the following:
https://github.com/CSBiology/DynamicObj/pull/38/files

Always use compiler directives for js and py only code, even on the lowest level. To still have intellisense, one can also include the !FABLE_COMPILER as an or case:

#if FABLE_COMPILER_JAVASCRIPT || FABLE_COMPILER_TYPESCRIPT || !FABLE_COMPILER

We use Thoth.Json in many places, would it be possible to add these directives to the js and py parts of the repo?

@njlr
Copy link
Contributor

njlr commented Sep 25, 2024

To still have intellisense, one can also include the !FABLE_COMPILER as an or case:

I think compiler directives can be set in the fsproj file so this is not necessary?
e.g. on your JS library you could add FABLE_COMPILER_JAVASCRIPT to the fsproj

@HLWeil
Copy link
Author

HLWeil commented Sep 25, 2024

Everything you put into the fsproj files alters the published nuget package. As Fable uses standard compilers like msbuild to resolve the dependency tree, all files and projects referenced using FABLE_COMPILER_JAVASCRIPT are left out and crash the transpilation downstream.

-> Transpiling a project with conditions works but downstream projects referencing the published nuget package will not be transpilable

@MangelMaxime
Copy link
Contributor

To still have intellisense, one can also include the !FABLE_COMPILER as an or case:

I think compiler directives can be set in the fsproj file so this is not necessary? e.g. on your JS library you could add FABLE_COMPILER_JAVASCRIPT to the fsproj

Indeed, we can set the compiler directives in the fsproj via DefineConstants property.


Hello,

I remember talking about this problem with @Freymaurer. I don't remember if this was somewhere in public or in private.

What is causing issues like you mentioned is the fact that you have a single Project that depends on different libraries for the different runtime:

  • DynamicObj
    • Thoth.Json.JavaScript
    • Thoth.Json.Python

This works fine for compiling DynamicObj and using its output.

However, you don't only expect to use the output of DynamicObj but you also want to use it as a dependency for others projects / .NET projects.

In theory there are 2 ways to solve this problem:

  1. Add top level compiler directives so no code is generated if the library is not compiled for the correct runtime (this is the solution you are proposing)
  2. Use 1 projects per target like Thoth.Json does (Elmish does something similar for Fable vs .NET usage)
    • DynamicObj.Python
    • DynamicObj.JavaScript

Personally, I prefer the second solution because it doesn't do anything strange to the dependencies. For example, DynamicObj package would need to have both a dependency on Thoth.Json.JavaScript and Thoth.Json.Python when only once is required in theory.

Another issue, I see is that it is possible to have a difference between the DLL API (used for intellisense) vs the actual API available for the runtime. In theory, it should not be too problematic because DLL API is a the same or at worth a subset of the target specific API available.

But it means that all the target specifics helpers that you put in place, cannot be accessed to the end user. The DLL compile only 1 versions of the code, it can't decide to expose the JS or Python APIs based on compilers directives. This would be possible if Fable, was able to hooks into the TargetFramework of .NET but the person wanted to do that does not seems active anymore and we didn't get an answer from NuGet/MSBuild team. (I tried to ask again for news on the subject).

Is there a reason why you can't use DynamicObj.Python, DynamicObj.JavaScript to offer dedicated projects to your customers?

After writing these notes, Fable is not the only one in a similar position. For example, Avalonia a cross platform UI solution has similar problems. They need to provide supports for iOS, Android, Desktop, and it seems like they do it via different packages:

They do have however, one benefit which is to have different frameworks registered on NuGet to target the runtimes too:

  • net8.0-android34.0
  • net8.0-ios17.0
  • net8.0-tvos17.0

@MangelMaxime
Copy link
Contributor

Another issue, I see is that it is possible to have a difference between the DLL API (used for intellisense) vs the actual API available for the runtime. In theory, it should not be too problematic because DLL API is a the same or at worth a subset of the target specific API available.

I think you don't see this issue because when compiling the DLL you will use dotnet pack and thanks to the rule !FABLE_COMPILER then the DLLs contains all the APIs of all the targets.

But this also means that if we were to set the DefineConstants in the fsproj then when compiling the DLL only a portion of the code would be available. In the case of Thoth.Json.XXX I think it doesn't cause issues because there is 1 target per package but in the case of something like DynamicObj this would causes issues.

I feel like not using one package per target, is creating a path where a lot of edge cases are pilling up.

@HLWeil
Copy link
Author

HLWeil commented Sep 26, 2024

Hey @MangelMaxime,

thanks for your detailed answer and thoughts about this. I agree with everything you said, but using one package per target comes with its own set of problems. Especially code duplication and maintainability of so many parallel packages.

I am currently try-forcing my approach in https://github.com/nfdi4plants/ARCtrl. For this, I copied the Thoth.Js and Thoth.Py code into https://github.com/fslaborg/FsSpreadsheet and added the compiler directive conditions.

If this works, I would like to merge the framework specific packages on an even lower level. I feel like having this functioning on a low level will make everything in the dependant packages easier.

Will keep you updated here, if you don't deem it too cluttering.

@MangelMaxime
Copy link
Contributor

Especially code duplication and maintainability of so many parallel packages.

It should not lead to more code duplication because you can re-use your sources files. Each project can include the sources files they need and a source file can be included several times.

This is what I am doing for Fable.Form, where I need to support several renderer library like React, Sutil, Fable.Lit, FuncUI, and it doesn't lead to more code duplication than if I was to inline everything in a single package.

Plus the DX, is easier I can switch across different solution files, to have the IDE/Packages setup needed for the target I want. If I want to work for Sutil, I open Fable.Form.Sutil.sln and then I get access to only Sutil related code.

Without that, if I wanted to develop a specific target I would need to either manually edit the DefineConstants and not forget to not commit it, or work without intellisense for some portion of the code because the compiler directives are not set up correctly.

By doing so, we are mimicking how IDE natively support MultiTarget framework (in their case via IDE GUI settings):

image

For this, I copied the Thoth.Js and Thoth.Py code into fslaborg/FsSpreadsheet and added the compiler directive conditions.

The problem is that now you are forking the ecosystem, in the linked PR I see fork of the following libraries:

  • Fable.Promise
  • Thoth.Json

@MangelMaxime
Copy link
Contributor

And there are others ways of supporting multi target project at least for Thoth.Json.

Work started to create a Thoth.Json.EveryWhere (need to find a name), which don't rely on a native parser but instead use a full F# parser based on Thoth.Parser.

This way people could use the native libraries if they care about performance or the custom parser for cross compatibility.

@MangelMaxime
Copy link
Contributor

Something I want to say, is that I have the tendency to be a purist or really safe when it comes to these decision because once we let the Jinn out of the bottle it is difficult to go back.

In theory, going the way you are asking to @HLWeil should not cause too much problem because I think the situation where there is a disconnect between the DLL information and the actual code available at runtime are problem invalid/non supported use cases.

For this reason, I am also asking for feedback from the F# community because this is something that don't only impact Thoth.Json but will probably shape some future libraries too because people with mimic what will be done here.

@MangelMaxime
Copy link
Contributor

For new people joining the discussion here is a summary.

To @HLWeil, the explanations below are adapted based on your situation but can vary a little for the sake for making the summary exhaustive.

The situation we are trying to solve is the following:

We have a library DynamicObj which targets .NET, JavaScript, Python.

  1. Compiling this library to native code via single project file for the different runtimes works, thanks to using MSBuild condition to only activate the dependency required for that runtime:
<PackageReference Condition="'$(FABLE_COMPILER_PYTHON)' == 'true'" Include="Thoth.Json.Python" Version="0.2.0" />
<PackageReference Condition="'$(FABLE_COMPILER_JAVASCRIPT)' == 'true'" Include="Thoth.Json.JavaScript" Version="0.1.0" />

Warning

However, it can lead to issues if you are using lock files because then depending on what target you are trying to build then the dependency list is not stable and so the lock file will change each time. Rendering the lock file not useful.

  1. Re-using the library from another F# project (we will call it my MyApp) causes issues because in these case if you use a single project file, then that library needs to depends on all the dependencies for all the target.

Meaning that DynamicObj will depends on:

  • Thoth.Json.Core (no problem this is a target agnostic library)
  • Thoth.Json.NewtonSoft
  • Thoth.Json.JavaScript
  • Thoth.Json.Python

Because of that, when compiling the MyApp to JavaScript or Python code, this leads to an issue because Fable will try to generate codes coming from the different targets. So you when compiling to Python, you end up with some JavaScript code in your generated files.

To solve this issue there are 3 visions:

  1. Mimic how TargetFramework works by creating multiple package specialised for each target:
  • DynamicObj.Net
  • DynamicObj.JavaScript
  • DynamicObj.Python

Pros

  • Use .NET dependency as usual
  • Package only include the dependencies that matter for the target
  • Thanks to the multiples .fsproj you get a normal IDE supports
  • Any APIs available is usable

Cons

  • You need to choose which version of the package you want to use (multiple fsproj)
  1. Add top level compiler directives around any code produces to prevent it from being use on an invalid target
namespace Thoth.Json.JavaScript

#if FABLE_COMPILER_JAVASCRIPT || FABLE_COMPILER_TYPESCRIPT || !FABLE_COMPILER
// your F# code goes here
#endif

Pros

  • A single package is generated

Cons

  • IDE support is limited (intellisense not available at some places) because it cannot know which compiler directives are set

    You can work around this limitation by manually editing your F# project, but you need to remember to remove the directives. Otherwise, you will generates an incomplete/invalid DLL.

  • The generated DLLs contains APIs that you can't use because it can for example contains low-level interop code for JavaScript, Python, etc. So even if an APIs is available thought intellisense it does not means that you can use it.

  1. Have TargetFramework specifics to Fable created by the NuGet team, so we you can hooks more into the normal way of doing it in .NET.

Pros

  • Should be standard to .NET

Cons

  • We need the NuGet team to do some work for us (we asked for support and are waiting for an answer) and we need to be sure that this can solve the issue.
  • Will it solve our problem? I think so, but I am no expert in the domain.

What are your thoughts on the problem and how would you like the problem to be solved?

PS: I tried to be objective when presenting the different situations but this is always a difficult exercise to do when you have an opinion. So please, be gentle in the comments 😇

@TheAngryByrd
Copy link

IDE support is limited (intellisense not available at some places) because it cannot know which compiler directives are set

Time to revive ionide/FsAutoComplete#1065

@MangelMaxime
Copy link
Contributor

We stumbled across a very intricate, general problem with Fable. The base problem is, that python fable transpilation fails, if emitted javascript code is referenced somewhere down the line.

@HLWeil You are saying that Python fable transpilation fails.

But do you have a similar problem when compiling to JavaScript ?

@Freymaurer
Copy link
Contributor

Hey Maxime! Thanks for your input! Maybe i can add some more information on why we have troubles following your suggested way "Mimic how TargetFramework works by creating multiple package specialised for each target".

One of the main goals of our library is to create js/py and dotnet code, which should all be easy to use from their respective native language. Therefore we have one main type which should be imported for js and py users (let's call it MainType). MainType includes a lot of logic using [<AttachMembers>]. Both logic for json/xlsx parsing and logic which is not framework dependant. If we want to create multiple packages we would need to duplicate at least the members of this type which are not framework dependant. This would be a non issue if fable was able to extend classes with [<AttachMembers>] and type extensions. Then we would be able to split the logic into multiple files. But there might be a better solution to this.

It should not lead to more code duplication because you can re-use your sources files. Each project can include the sources files > they need and a source file can be included several times.

This is what I am doing for Fable.Form, where I need to support several renderer library like React, Sutil, Fable.Lit, FuncUI, and it > doesn't lead to more code duplication than if I was to inline everything in a single package.

Plus the DX, is easier I can switch across different solution files, to have the IDE/Packages setup needed for the target I want. If I > want to work for Sutil, I open Fable.Form.Sutil.sln and then I get access to only Sutil related code.

Can you point me to the relevant code here? I looked over the Fable.Form repo but could not find it 😅

(Thanks for all your work again 🙂 )

@HLWeil
Copy link
Author

HLWeil commented Sep 27, 2024

@HLWeil You are saying that Python fable transpilation fails.

But do you have a similar problem when compiling to JavaScript ?

Sry that was not well put. From what I've seen, the JavaScript transpiler is more forgiving towards Python code then vice versa, but still not reliably.

@MangelMaxime
Copy link
Contributor

@Freymaurer @HLWeil

I made an example repo to demonstrate how it would works. If you want we could have a call so I explain it to you, and perhaps we can adapt it to your project.

https://github.com/MangelMaxime/fable-example-multi-targets

As statement in the README, I have an issue running the generated Python code but I don't know how to fix that issue.

@MangelMaxime
Copy link
Contributor

Closing as we decided to go with the multi projects approach.

@HLWeil
Copy link
Author

HLWeil commented Oct 2, 2024

Implementation of the discussed concept can be seen in nfdi4plants/ARCtrl#446.

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

5 participants