Skip to content

When watching failed lookups, watch packageDir if its a symlink otherwise the path we use to watch #58139

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

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

Image for: Conversation
Copy link
Member

sheetalkamat commented Apr 9, 2024

In mono repo scenarios the packages are linked using symlink.
So when trying to resolve package1 from package2 in the test we look for locations like: /home/src/projects/project/node_modules/package1/dist/index.d.ts

As a result when package1 fails to resolve, we want to watch if this path will exists.
Before this PR, we would be watching /home/src/projects/project/node_modules as its a node_modules folder and we dont want to be watching too many packages or folders. This results in issues because /home/src/projects/project/node_modules/package1 is a symlink and any changes to the target are not percolated to the watcher we create. So without this PR we would not see the changes and miss the package build and report errors about package1 not found and never be able to resolve that unless offcourse if you start fresh by restarting tsserver

Now when we calculate dir to watch, we also find the packageDir if this happens to be symlink, we will watch it otherwise we will watch the node_modules like we use to watch before.

Other change needed for this to work on linux was that when we are watching this symlink dir, we need to ensure we are watching the target recursively as that is the expectation for native watchers that support symlink watching.

We already had test case which didnt update the diagnostics on tsserver when paths was not used and with my other prototype changes to treat file create and delete as change and retain things more, it becomes more evident and those tests just start to fail. So this is cruicial fix for that change to go in as well

Fixes #55450

@@ -272,7 +272,7 @@ FsWatches::
{}

FsWatchesRecursive::
/user/username/projects/myproject/node_modules: *new*
/user/username/projects/myproject/node_modules/@issue/b: *new*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these new watchers, my impression is that they are attempting to watch the future linking or changing of /user/username/projects/myproject/packages/B, right? Does the case matter here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casing is handled. Its not target we are watching but symlink so this is something we lookedup as part of failed lookup and we are watching that.

sheetalkamat merged commit b006768 into main Apr 12, 2024
sheetalkamat deleted the siblingSymlinks branch April 12, 2024 18:03
Copy link

Thank you very much for your work on this! Highly appreciated 🎉

Copy link

r0ssIV commented May 29, 2024

@sheetalkamat could you update when this fix will be released in typescript?

Copy link
Member Author

This is already in 5.5 beta

Copy link

r0ssIV commented Jul 5, 2024

@sheetalkamat So this fix is already in prod?
Just switched to TS 5.5.3

Quick question: does it work only with .ts files imported from symlinked folder, or any .js files (for example from node_modules/@libraryname/{some_symlinked_folder}/dist/some_compiled_lib.js ?

Still having the issue with VSCode. One of my node_modules packages - node_modules/@libraryname/package is a symbolic link to another folder. If files are changed in that folder, should it detect changes?

P.S.: checked and it still does not, I need to call restart TS server in VSCode

Copy link
Member Author

This is in 5.5
If you are running into issues, pls file issue with logs and way to repro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
5 participants