Revist logging

[nojaf]

Currently, a lot of logging happens directly to the standard output.
This is unwanted in the Fable.Compiler, and I wish to introduce a logging abstraction.
Inside my use case, I would like to log in to memory or disk.

The only thing I'm not quite sure about is what to do with #2727. It seems like some code was written to avoid this problem and I'm not sure it will still be problematic after this change.
@AngelMunoz, is this still an active use-case of yours?

[MangelMaxime]

Reading the linked issue in your command, ringed a bell to me.

I believe we still have some issues when running Fable in watch mode.

I don't know if this is related to the logger or not but, when running a sub process via Fable sometimes the processes are freezing.

Example of command: dotnet fable --run npx vite

#3631

I observed a similar behaviour when working on fantomas-tool or even worth sometimes for that particular project where vite process would start and just stop completely at some point.

[AngelMunoz]

@nojaf The main problem there was that the logs were being re-written in the same line via cursor manipulation, this could cause hang ups if the output was redirected (e.g. calling Fable via Process APIs or in my case CLIWrap).

From a quick glance on this PR, looks like the main change is that logging now goes through an ILogger right?

I'm guessing that leaves using the cursor to write in the same line/position out of the window, if that is the case, I believe it should not break anyone calling fable from another process via stdio.

[nojaf]

Ok, thanks for confirming. For now, I think I'd like to remove that cursor manipulation thing, as it is probably tricky to achieve using the ILogger.

[MangelMaxime]

The cursor manipulation allows for a much smaller / easier to read output in the log when Fable does all the recompilation of the files. So it would be nice to keep it.

There is currently the --verbose flag which I believe allow people to disable it if it needed.

[nojaf]

Ok, I restored that cursor update functionality.
Ready for review.

[MangelMaxime]

I don't have much time this week, review will most likely happens next week.

[ncave]

@nojaf Minor CI error to fix:

Error: /home/runner/work/Fable/Fable/tests/Integration/Compiler/Util/Compiler.fs(36,9): error FS0764: No assignment given for field 'Verbosity' of type 'Fable.Compiler.Util.CliArgs'

[github-actions]

Failed to run : https://github.com/fable-compiler/Fable/actions/runs/7291903983

[nojaf]

Minor CI error to fix:

Thanks, is this project not part of the Fable.sln?

[ncave]

@nojaf

is this project not part of the Fable.sln?

I think it is.

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[nojaf]

Alright, another crucial part of Fable.Compiler is that no code writes to stdout!
This messed up the JSON RPC communication I'm using for the Vite plugin.

To guarantee that nobody ever sins against this, I've created an analyzer that detects calls to prinfn and Console.Write. The results are logged here. And it is possible to configure a PR build to fail if errors are present. (Needs to be configured in GitHub).

build / analyzers is green because the process of running the analyzers succeeded. This won't fail if the analyzers detect something bad.

Code scanning results / Ionide.Analyzers.Cli is green because no new code violated the rules. In future PRs, we want this to be red when someone breaks the rules.

I still need to address the actual Console.Write usage in Fable.Transform, so please don't merge this PR yet. No need to convert to draft, I should be able to address this in the next days.

[ncave]

@nojaf

I still need to address the actual Console.Write usage in Fable.Transform

I removed the Console.WriteLine usage out of Fable.Transforms in #3704, please rebase your PR.
You can now move the Log module out of Fable.Transforms and back to Fable.Compiler.
This should fix your Fable.Standalone build issue.

[nojaf]

@ncave thanks! I've rebased and all is green now.

@MangelMaxime with the getPlugin changes in here I'd like to have this PR merged.
I was able to use my local Vite plugin for a small Fable project I have.
It would be great to have another Fable.Compiler release with this.

[MangelMaxime]

@MangelMaxime with the getPlugin changes in here I'd like to have this PR merged.

Noted, I will work on the next release of Fable.

It would be great to have another Fable.Compiler release with this.

To trigger the release for Fable.Compiler can you please update its Changelog file? You can update the Unreleased section for now, I will bump the version when doing the release process.

If you could update the Unreleased section of the different Changelog files impacted by this PR it would be great 🙏

[nojaf]

Done!

[MangelMaxime]

Thank you for the contribution and to the reviewers