Fix module resolution across multiple dependent workspaces with incompatible tsconfigs

[dbudzins]

Still draft until I can figure out why some tests are failing on github but not local.

This PR is my attempt to fix #610.

• Do not save module resolution failures in the resolutionCache of resolveModuleNames.ts so that a failed attempt to resolve a file in the wrong principal doesn't block resolution in the right principal from succeeding
• Analyze scripts rescursively in main analysis loop so they can be handled within the principal they belong to and don't get mixed up with the internalWorkspaceFilePaths
• Analyze all internalWorkspaceFilePaths after analyzing all of the principals and lookup which principal each belongs to
• Small adjustments to the way analyzed files are excluded from repeat processing to simplify code changes above

[webpro]

Thanks for the PR! I really appreciate you went deep into the codebase. I think you're on to something and this might fix some nasty bugs I wasn't aware of indeed.

Still draft until I can figure out why some tests are failing on github but not local.

This PR is my attempt to fix #610.

• Do not save module resolution failures in the resolutionCache of resolveModuleNames.ts so that a failed attempt to resolve a file in the wrong principal doesn't block resolution in the right principal from succeeding

The idea is that --isolate-workspaces forces one principal per workspace. Did you see and try this flag? Ideally we don't need it at all, just good to know it's available. Hopefully after this PR this flag becomes even less useful :)

Yet this "isolation" isn't/can't be enforced towards TypeScript itself (i.e. starting at TS.createProgram()), so the module resolver needs to handle this. So yeah, this is definitely a good idea.

• Analyze scripts rescursively in main analysis loop so they can be handled within the principal they belong to and don't get mixed up with the internalWorkspaceFilePaths
• Analyze all internalWorkspaceFilePaths after analyzing all of the principals and lookup which principal each belongs to

Not sure why this is regarded as "mixed up". The goal of internalWorkspaceFilePaths is to prevent infinite recursion for circular import chains. Yet perhaps with the early analyzedFiles.has(filePath) we can now get rid of internalWorkspaceFilePaths altogether? This would be great, guess it evolved this way with your PR and one or more refactorings recently.

The factory.deletePrincipal is now also deferred to the end and this is something I'm less fond of. This is something we should try to do within the main principals iteration, since they tend to consume a lot of memory in large monorepos. Without internalWorkspaceFilePaths this seems feasible again?

• Small adjustments to the way analyzed files are excluded from repeat processing to simplify code changes above

[webpro]

we can now get rid of internalWorkspaceFilePaths altogether?

Sorry, scratch that. After thinking it through a little bit more, maybe this is a more viable approach:

• Never cache failed attempts in module resolver.
• Use isolateWorkspaces for your changes:
  • false (default): keep behavior as it is in main
  • true: go the "safe but memory heavy" route, basically your solution in this PR

So behavior won't change much for existing users, and your case should be solved by adding the --isolate-workspaces flag that will split up the workspaces and keep them in memory as well to finish up (cross) module resolution properly. Wouldn't impact this PR that much it seems. What do you think?

Maybe the default should be swapped in a next major. The thing is, memory consumption can grow huge easily, and not allowing the garbage collector to clean up makes knip crash in some projects. That's not a great default experience either. Unfortunately I miss data to make a better informed decision here. All I got were crash reports I couldn't reproduce but they were fixed in the default mode (I'm afraid this PR would make those projects crash again).

[dbudzins]

I haven't tried the isolate flag on my simple repro, but for my real use case it didn't help (without these additional changes) because referenced files from other workspaces were still pulled in by parent workspaces where they were used.

Unfortunately my approach does indeed rely on keeping the principals in memory the whole time since a workspace might add a file that needs to be analyzed by a principal that has previously been analyzed. I'm not sure there's any other way to do it, at least not without major refactoring.

I agree that it might make sense to take the approach of 2 different behaviors with and without the isolate filag. I'll try that out and see how it goes. Thanks for the quick review and nice conversation! Happy to help out, as this project has been a helpful tool!

[webpro]

That's great to hear. Thanks for working on this! It was not an intuitive thing to do, and by default leads to surprising/incorrect behavior, but the performance/memory improvements by combining workspaces into single programs when possible was really significant.

[webpro]

@dbudzins Just curious: is this still something you fancy working on?

[dbudzins]

It is, but I'm really stuck. Fixing this breaks other tests, and I'm not really sure what to do. Have you had any additional thoughts on how to best solve it?

…

On Sun, May 19, 2024, 08:28 Lars Kappert ***@***.***> wrote: @dbudzins <https://github.com/dbudzins> Just curious: is this still something you fancy working on? — Reply to this email directly, view it on GitHub <#611 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYTA66JGPQTHDUMNY6XCELZDBBBNAVCNFSM6AAAAABG7GXZSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGEYTSNZWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[webpro]

I'll look into it and give it a shot today or tomorrow, and report back :)

[webpro]

Today I asked since I was working on something related and wanted to make sure things are still compatible.

Now I've looked into this and I've pushed a branch with all the latest stuff that has things running fine for me.

The commit dd97ca0 is something you can cherry-pick on top of your branch to isolate and try out my changes on top of this PR. The main thing is isIsolateWorkspaces in src/index.ts.

It's still a bit messy, but let's first see if this holds up in your real use case. You probably should add the --isolate-workspaces flag.

[webpro]

Just in case you were looking at it, I just did a major refactoring and pushed it to the other to-src-path-and-fix-ws-resolution branch. Sorry it's in flux this much, but I'm happy where its going. You could try that if you're curious, results should improve, otherwise I'm happy to hear about remaining issues.

[webpro]

I've merged this to make sure you're the contributor. I'll just commit more on top of it in main. Thanks a lot @dbudzins, it has been super helpful!

[webpro]

🚀 This pull request is included in v5.17.0-canary.0. See Release 5.17.0-canary.0 for release notes.

Using Knip in a commercial project? Please consider sponsoring me.

[webpro]

Would be great if you could try this out on your project:

npm install -D knip@canary

[dbudzins]

@webpro I tried out the latest changes from your branch and both the test fixture and my real life use case pass successfully with the --isolate-workspaces flag. Sorry I couldn't figure out the refactor, but I'm glad you got it working! Thanks for the help!

[webpro]

Awesome, thanks for the feedback and all the energy that went into this!

[webpro]

The latest canary includes another refactoring, making --isolate-workspaces no longer required in more situations.