Skip to content

Tweak module resolution failed lookup watching #53420

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

Closed
wants to merge 8 commits into from
Closed

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Mar 21, 2023

This simplifies unnecessary extension watching from long ago when we had different resolutions based on .ts and .js files
Before we would never watch directories like c:/typescript/node_modules or /user/username/typescript/node_modules because we didnt watch folders like c:/typescript or /user/username/typescript. Now changed to watch node_modules in root folder. (We wont watch it at root unless tsconfig is at the root)

This should help with some of the npm ci issues in repo encountered if typescript repo was at root or in another folder .

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 21, 2023
@sheetalkamat sheetalkamat requested a review from jakebailey March 21, 2023 19:54
@sheetalkamat sheetalkamat marked this pull request as draft March 21, 2023 19:58
…eself instead of its directory

So with this before we use to ignore c:/typescript/node_modules watch as we will never watch c:/typescript now it will
@sheetalkamat sheetalkamat changed the title Correctly update the ts, js extensions to handle when watching failed lookups so we are not doing extra tracking of actual paths Tweak module resolution failed lookup watching Mar 22, 2023
@sheetalkamat sheetalkamat marked this pull request as ready for review March 22, 2023 19:01
Comment on lines +46 to +49
PolledWatches::
/node_modules/@types: *new*
{"pollingInterval":500}

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why these were missing before and are added now? I don’t quite see why from the implementation change.

Copy link
Member Author

@sheetalkamat sheetalkamat Mar 24, 2023

Choose a reason for hiding this comment

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

Sorry i thought this was also handled by "canWatchFileOrDirectory" thing but we had logic for watching node_modules/@types at tsconfig or jsconfig directory as well which has some issues when it is /. I will look more into this and fix any other issues with config at root directory.

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 24, 2023

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f64f2ad. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 24, 2023

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/150952/artifacts?artifactName=tgz&fileId=44CA3A0E1125714E6701D3D003A9E7B950EED15E8D0F039F858E806CF328726202&fileName=/typescript-5.1.0-insiders.20230324.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@sheetalkamat
Copy link
Member Author

Closing this in favor of #53591 which i am still working on

@sheetalkamat sheetalkamat deleted the extensions branch March 30, 2023 17:27
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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants