-
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
Experiment with MSBuild DesignTime #3721
Conversation
Cracking at first glance worked on my end. I'll take a look at this later today. PS: A quick easy way to find out if cracking at least worked is by using this script. |
Just update the source to use the full path. If you can easily try it, give it a go I'd say. |
@MangelMaxime I couldn't help myself, after this last change |
There still are some gaps in this new resolver mechanism. |
Thank you for looking into it. It seems like this is working for some scenario like the Seems like this is not a as easy quick win as I was thinking, I think for today I will release a new version based on #3720 which workarounds the limitation we have with the current system. But I think we should continue to push the MSBuildDesign time work as this will hopefully be more robust and remove the workaround that we need to introduce ATM. If I can help we anything please tell me. |
Yup, this still needs some more love. Once we figured everything out for Fable itself and added some caching, we should play around with the feature toggle. Allow users to switch between this new thing and Buildalyzer. |
I updated the PR to change the name of the cracker file and also to include the latest changes I introduced in main. |
src/Fable.Cli/Main.fs
Outdated
@@ -971,6 +976,10 @@ let private checkRunProcess (state: State) (projCracked: ProjectCracked) (compil | |||
| _, exeFile -> exeFile, runProc.Args | |||
|
|||
if Option.isSome state.Watcher then | |||
printfn "cliArgs.RunProcessEnv: %A" cliArgs.RunProcessEnv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, I think it is me debugging something (if it me who added this code).
I think you also mentioned that for Vite we should not write directly to the console as it will mess up communication between processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well in Fable.Cli
that wouldn't be a problem as that doesn't end up in the Vite plugin.
But since we introduced the logger, it feels wrong to ever use printfn
again.
The lines are yours, I would remove them if we no longer need them.
@MangelMaxime I think this is in a good state now to release behind an experimental flag. |
Sorry didn't see that CI was green now. Yes we can release it by introducing a new Flag to Fable CLI to control the switch between We could do it with something like:
What do you think? Once we decide a the flag, if you want I can adapt the PR for it. |
The F# compiler prefixes its test arguments with |
Yes we can follow F# compiler naming and the suggested flag looks good to me. |
Add a CLI flag for this, went with I also didn't include this property in Feel free to tweak things here. |
Thank you. In reality, the casing is only respected when printing the help message. Because when looking for the value in the arguments list, the code Lines 41 to 45 in df40a18
We will handle that when rewriting Fable.Cli. With the number of things that we are starting to accumulate on that subject, I think it will be my next big task for Fable. |
This PR is based on #3720
@nojaf I tried to have a look at integrating MSBuild DesignTime instead of BuildAnalyzer.
But it seems like we are doing something incorrectly because it is not able to compile
Fable.Library.fsproj