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

Re-compile deleted files after project changes in watch mode #3684

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

OrfeasZ
Copy link
Contributor

@OrfeasZ OrfeasZ commented Dec 29, 2023

This is a fix for issue #3641, where Fable (in watch mode) will delete the files in fable_modules after a watched project file (.fsproj) changes, but won't recompile them after populating them again.

I'm not sure if this is the best way to do this, since @MangelMaxime mentioned that it might be better to not delete these files to begin with instead, but I did notice there was a TODO around this code so decided to implement it this way instead.

@MangelMaxime
Copy link
Member

Thank you for the PR.

I took some time to look why fable_modules is deleted and this is because of the changes I made to improve or change (choose you option) how Fable cache works.

The new cache mechanism is a bit more agressive in the sense that if it sees Fable compiled with --noCache or the cache was found invalidated then it will delete fable_modules to start from a fresh state.

In the past, it was working because Fable was much less agressive but this is what leads to some scenario where you would see several version of the same library inside of fable_modules.

I tried to adapt the new cache system to detect when Fable triggers the process from watching the fsproj changes but in the end we still end up in the state where Fable consider the cache invalid meaning that we probably need to clean it.

Because I think that the files are kept in memory inside of Fable or FCS, recompiled the whole fable_modules seems fast enough to me. So going with the changes you made seems to be alright.

If we need to revisit this problem in the future here are the places where we need to look:

  1. This is where Fable trigger a re-evaluation of the project when the fsproj file change.

    Fable/src/Fable.Cli/Main.fs

    Lines 1232 to 1233 in e7d45db

    let newProjCracked =
    ProjectCracked.Init({ cliArgs with NoCache = true })

    Currently, Fable force the NoCache = true, but what I explored was to add an optional argument TriggeredFromWatcher to ProjectCracked.Init to differentiate from NoCache and re-evaluation

  2. Here in when getting fable_modules, we could say that we delete folder only if not coming with triggeredFromWatcher = true

    static member GetFableModulesFromProject
    (
    projDir: string,
    outDir: string option,
    noCache: bool
    )
    : string
    =
    let fableModulesDir =
    outDir
    |> Option.defaultWith (fun () -> projDir)
    |> CrackerOptions.GetFableModulesFromDir
    if noCache then
    if IO.Directory.Exists(fableModulesDir) then
    IO.Directory.Delete(fableModulesDir, recursive = true)
    if File.isDirectoryEmpty fableModulesDir then
    IO.Directory.CreateDirectory(fableModulesDir) |> ignore
    IO.File.WriteAllText(
    IO.Path.Combine(fableModulesDir, ".gitignore"),
    "**/*"
    )
    fableModulesDir

  3. Lastly, here this is where the cache is more agressive than before.

    // The cache was considered outdated / invalid so it is better to make
    // make sure we have are in a clean state
    opts.ResetFableModulesDir()

    Because, the cache is considered invalid in our case because the new fsproj is newer than the previous version then we delete the fable_modules folder. That's the place where I could not find a way to make the cache less aggressive and like I mentioned I am not sure that we want to make it less aggressive here.

@MangelMaxime MangelMaxime merged commit cc06404 into fable-compiler:main Jan 24, 2024
16 checks passed
@lukaszkrzywizna
Copy link

lukaszkrzywizna commented Jan 25, 2024

@OrfeasZ @MangelMaxime thanks for the PR. I've tested it with my project and encountered an issue with Thoth.Elmish.Debouncer. Initially, after the first compilation, the import paths are correct and look like this:

import { Union, Record } from "../fable-library.4.10.0/Types.js";
import { union_type, record_type, class_type, int32_type, string_type } from "../fable-library.4.10.0/Reflection.js";
import { remove, add, tryFind, empty } from "../fable-library.4.10.0/Map.js";
import { comparePrimitives } from "../fable-library.4.10.0/Util.js";
import { some, map, defaultArg } from "../fable-library.4.10.0/Option.js";
import { PromiseBuilder__Delay_62FBFDE1, PromiseBuilder__Run_212F1D4B } from "../Fable.Promise.3.2.0/Promise.fs.js";
import { promise } from "../Fable.Promise.3.2.0/PromiseImpl.fs.js";
import { Cmd_none, Cmd_OfPromise_either } from "../Fable.Elmish.4.1.0/cmd.fs.js";
import { singleton } from "../fable-library.4.10.0/List.js";

However, after any changes to the .fsproj file, when fable_modules are rebuilt, the imports change and seem incorrect:

import { Union, Record } from "../demo/fable_modules/fable-library.4.1.3/Types.js";
import { union_type, record_type, class_type, int32_type, string_type } from "../demo/fable_modules/fable-library.4.1.3/Reflection.js";
import { remove, tryFind, add, empty } from "../demo/fable_modules/fable-library.4.1.3/Map.js";
import { comparePrimitives } from "../demo/fable_modules/fable-library.4.1.3/Util.js";
import { some, map, defaultArg } from "../demo/fable_modules/fable-library.4.1.3/Option.js";
import { Cmd_none, Cmd_OfPromise_either } from "../demo/fable_modules/Fable.Elmish.4.0.1/cmd.fs.js";
import { PromiseBuilder__Delay_62FBFDE1, PromiseBuilder__Run_212F1D4B } from "../demo/fable_modules/Fable.Promise.3.2.0/Promise.fs.js";
import { promise } from "../demo/fable_modules/Fable.Promise.3.2.0/PromiseImpl.fs.js";
import { singleton } from "../demo/fable_modules/fable-library.4.1.3/List.js";

Additionally, the corresponding .fs.js.map file is not being rebuilt.

@MangelMaxime
Copy link
Member

MangelMaxime commented Jan 25, 2024

Hello @lukaszkrzywizna,

The imports being incorrect is because I included the generated Fable files in the package.

https://github.com/thoth-org/Thoth.Elmish.Debouncer/blob/e736a7581c73734d69f1a0593e85b3d22de1be18/src/Thoth.Elmish.Debouncer.fsproj#L20

I believe the .fs.js.map not being rebuilt is due to the same problem. I think because Fable see existing .js files then it thinks that nothing needs to be done for this file.

I released version 2.1.0 of Thoth.Elmish.Debouncer. It seems to have fixed the problem for me in my project. Can you please check on your side too?

@lukaszkrzywizna
Copy link

@MangelMaxime It works. Big thanks 🙇

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

Successfully merging this pull request may close these issues.

3 participants