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

Fix module resolution across multiple dependent workspaces with incompatible tsconfigs #611

Merged
merged 4 commits into from
May 22, 2024

Conversation

dbudzins
Copy link
Contributor

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

@dbudzins dbudzins changed the title Fix #610 Fix module resolution across multiple dependent workspaces with incompatible tsconfigs Apr 29, 2024
@webpro
Copy link
Collaborator

webpro commented Apr 30, 2024

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
Copy link
Collaborator

webpro commented Apr 30, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

webpro commented Apr 30, 2024

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
Copy link
Collaborator

webpro commented May 19, 2024

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

@dbudzins
Copy link
Contributor Author

dbudzins commented May 19, 2024 via email

@webpro
Copy link
Collaborator

webpro commented May 19, 2024

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

@webpro
Copy link
Collaborator

webpro commented May 19, 2024

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
Copy link
Collaborator

webpro commented May 21, 2024

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 webpro marked this pull request as ready for review May 22, 2024 21:14
@webpro webpro merged commit f99e5f1 into webpro-nl:main May 22, 2024
3 of 14 checks passed
@webpro
Copy link
Collaborator

webpro commented May 22, 2024

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
Copy link
Collaborator

webpro commented May 22, 2024

🚀 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
Copy link
Collaborator

webpro commented May 22, 2024

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

npm install -D knip@canary

@dbudzins
Copy link
Contributor Author

@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
Copy link
Collaborator

webpro commented May 22, 2024

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

@webpro
Copy link
Collaborator

webpro commented May 26, 2024

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

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.

Module Resolution Issues Across Multiple Workspaces with TSConfig Paths
2 participants