fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
273 stars 67 forks source link

ReactComponent Attribute #207

Closed alfonsogarciacaro closed 3 years ago

alfonsogarciacaro commented 3 years ago

This PR supersedes #196 and uses the plugin system from Nagareyama instead (see also https://github.com/fable-compiler/Fable/pull/2208). The code for the plugin is a bit more convoluted because the whole Fable.AST is now exposed. I haven't published the internal helpers for AST manipulation because the API wasn't designed for public consumption and it's a bit messy. For now I just added some simple helpers in the AstUtilmodule.

The workflow is as follows. I hope it's good enough but we can try to improve it if needed:

Development

During development you don't want to republish the package to test every change. In that case, use a project reference (see TodoMVC.fsproj in this pr) together with the --exclude option, which instructs F# to ignore the sources from referenced projects and use the compiled .dll assemblies instead. For this, make sure you build the project containing the plugin before starting Fable. The steps would be:

dotnet build my/plugin.project
dotnet fable my/test.sample --exclude plugin.project

You can see a more complex example of this in the new npm todomvc-start script: https://github.com/fable-compiler/fable-react/blob/9bc42fb37aa77695b0d2cca3455b1d41ab633d56/package.json#L5

@Zaid-Ajaj I hope this is a good guide to start hacking with the AST. Please give it a try when you have a moment and let me know how it goes.

Zaid-Ajaj commented 3 years ago

Awesome stuff @alfonsogarciacaro, I will be diving into it very soon, probably after this weekend. I have one question about

Plugins must go in dll assemblies that are not compiled by Fable. Basically this means they go in a separate package (you can still reference it from your Fable library) and not include the sources (so a tag like this is not necessary).

Does that mean I cannot implement a plugin in the library itself (say Feliz) but instead would require to implement a separate package (like Feliz.Plugin) where the plugin is implemented?

alfonsogarciacaro commented 3 years ago

Yes, sorry. I did try putting the plugin in the library itself at first but had issues when Fable tried to compile the plugin code. Putting the plugin into a separate assembly/package has some advantages (clear distinction between plugin and to-be-js-compiled code, Fable compilation has less work to do, you can use .NET APIs), but if it becomes too cumbersome we can try some work around, like including the Fable.AST sources to make it Fable-compatible or erase the plugin code somehow. Though in that case we may have some challenges to locate the .dll containing the plugin, particularly during development.

Zaid-Ajaj commented 3 years ago

Yes, sorry. I did try putting the plugin in the library itself at first but had issues when Fable tried to compile the plugin code. Putting the plugin into a separate assembly/package has some advantages (clear distinction between plugin and to-be-js-compiled code, Fable compilation has less work to do, you can use .NET APIs)

Doesn't adding !FABLE_COMPILER is enough to tell Fable not to compile the plugin code? At the end of the day, when we are publishing the nuget packages, we are not defining the FABLE_COMPILER constant which means:

Another thing just to double check: consumers of libraries with plugins don't need to exclude the plugin from the compilation, correct? Only developers of the library need to use the --exclude option if I understand correctly

alfonsogarciacaro commented 3 years ago

Forgot to comment about conditional compilation. Fable compilation has two phases: FCS compilation to generate the F# AST, and Fable compilation to translate the AST to Fable -> Babel (and now also to print the JS code). If you hide the attribute definition behind !FABLE_COMPILER then FCS will fail when using the attribute because the compiler doesn't know anything about it. You would have to hide only the transform code like:

type ReactComponentAttribute() =
    inherit MemberDeclarationPluginAttribute()
    override _.FableMinimumVersion = "3.0"
    override _.Transform(logger, decl) =
#if FABLE_COMPILER
       failwith "Not supported"
#else
       ...
#endif

And you also need to hide code used by the plugin like the AstUtil module so it can become complicated quite quickly. And we still have the problem of locating the generated (if it does exist) .dll from the source file during development.

consumers of libraries with plugins don't need to exclude the plugin from the compilation

That's right. For consumers everything will be transparent. They don't even need to install Feliz.Plugins because it will be a transitive dependency of Feliz. Fable will automatically skip merging the sources of packages if it doesn't find a fable folder in the Nuget package. --exclude will only be need during development if you use project references (I'm thinking now of a way to automatically rebuild the dll in that case). We do the same when compiling Fable.Library or the Fable tests when referencing the Fable.Core project directly: https://github.com/fable-compiler/Fable/blob/3c51973fb030aa1f23c1a008d470c5d7cbcc611d/build.fsx#L81

If you use local Nuget packages when developing you shouldn't even need to use the --exclude option (only --forcePkgs to clear the cached sources every time).

Zaid-Ajaj commented 3 years ago

Thanks a lot @alfonsogarciacaro for the thorough explanation 🙏 I will start working on the plugin as soon as time permits

alfonsogarciacaro commented 3 years ago

3.0.0-nagareyama-alpha-014 will automatically build the excluded project references, so the command has been simplified to:

cd Samples/TodoMVC
dotnet fable watch src --noCache --exclude Fable.React.Plugins --run webpack-dev-server
nojaf commented 3 years ago

Hey @alfonsogarciacaro, any news on this PR? As I'm using the original Elmish DSL in all my projects and I'm interested in react-refresh I'm really looking forward to this plugin. Any chances we can get an alpha/beta of this?

alfonsogarciacaro commented 3 years ago

Hi @nojaf! I was a bit concerned that if we publish two libraries with a similar ReactComponent attribute it could be a bit confusing for users, especially those mixing Fable.React and Feliz DSLs in their apps. So for now I was thinking to let users try @Zaid-Ajaj version and see how it works. In fact, you should be able to use Feliz.CompilerPlugins (which is already published) in your Fable.React apps without the Feliz dependency. Can you please give it a try and let us know if you have any problem?

nojaf commented 3 years ago

Hey @alfonsogarciacaro, that totally worked! Many thanks for pointing this out.

alfonsogarciacaro commented 3 years ago

Closing as we're recommending Feliz.ReactComponent instead.

kerams commented 2 years ago

@alfonsogarciacaro, has the development workflow changed? My plugin does not seem to be loaded. --exclude and ScanForPlugins are present, yet the annotated entities don't seem to trigger the compiler.LogWarning in Transform and TransformCall. The project compiles as if the attributes were not present.

alfonsogarciacaro commented 2 years ago

@kerams Did you add the ScanForPlugins assembly attribute? See this. Do you see a message like "Loaded plugin" when starting compilation?

kerams commented 2 years ago

Yes, as far as I can tell I followed the instructions to the letter. I only see loaded messages for Feliz plugins. Does the TFM matter? FSharp.Core, Fable.Core, Fable.AST versions in relation to the version of the Fable tool?

The plugin project is being compiled in .NET first, so that part appears to be working.

kerams commented 2 years ago

@alfonsogarciacaro, damn, I've just noticed Also, please put the plugins directly in a namespace and not inside a module.. That was the problem. Thank you.

(Hm, I've also now found out that MemberDeclarationPluginAttribute has no effect when placed on types, so I can't proceed anyway 😸)

alfonsogarciacaro commented 2 years ago

Ah, yes. I was planning to add plugins for classes too but I haven't done it yet because the need for it hadn't appeared yet 😅

chkn commented 9 months ago

Ah, yes. I was planning to add plugins for classes too but I haven't done it yet because the need for it hadn't appeared yet 😅

@alfonsogarciacaro I might have a need for this coming up. Has there been any other thought/discussion/work on this, or should I just take a stab at it and send a PR?

MangelMaxime commented 9 months ago

@chkn I think for now, the first step is to open an issue with what you are trying to archive, plus reproduction/pseudo code if possible

Like that maintainers will be able have a look at the use case, and provide feedbacks etc.