Skip to content

Module resolution should not look for node_modules outside project roots #52637

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

Open
sandersn opened this issue Feb 6, 2023 · 7 comments
Open
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@sandersn
Copy link
Member

sandersn commented Feb 6, 2023

Currently, module resolution looks for node_modules all the way to the root of the filesystem. However, this is slow for certain virtual filesystems, like the one used in vscode-web, which maps only a part of the file system to in-memory files and the rest to web requests. microsoft/vscode#173591 shows the workaround needed to stop tsserver from reading these parts of the filesystem.

Instead, module resolution should not look for node_modules directories outside project roots -- at least as specified in updateOpen messages, but I think there are other ways too.

@fatcerberus
Copy link

Hmm, this is interesting. While I agree with the basic principle, it’s technically a deviation from runtime behavior, wherein node will happily drill all the way up to the root looking for node_modules

@sheetalkamat
Copy link
Member

It also deviates from result you would get on commandLine vs in editor. So this will create discrepancies in errors if any.

@IllusionMH
Copy link
Contributor

I think this is useful behavior for JS projects where you need specific (major) versions of @types/* packages but don't want/can't add them to the package.json. It's possible to just have separate package.json in project's parent folder and it just works and I'm not sure how to achieve same behavior in other way.

Not sure if it is common use case, but something similar might be in monorepo where you open only specific package instead of the root.

@andrewbranch
Copy link
Member

It might be interesting to see how much of a perf win it would be to set this as a compiler option. Even in a program with no errors, there are often many failed package lookups, e.g. for Node built-in modules that end up being fulfilled by ambient module declarations.

@andrewbranch andrewbranch added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Feb 9, 2023
@DanielRosenwasser
Copy link
Member

There are situations when you really need the current behavior or else things would get weird. For example, let's say you clone cool-project whose directory contents look like this after running your setup scripts:

cool-project/
├── projects/
│   ├── front-end/
│   │   ├── src/
│   │   └── node_modules/
│   ├── back-end/
│   │   ├── src/
│   │   └── node_modules/
│   └── shared/
│       ├── src/
│       └── node_modules/
└── node_modules

Let's say that top-level node_modules comes from hoisted dependencies due to your package manager/monorepo management tool.

If you open your editor at front-end, then you simply won't resolve anything from node_modules, right?

Even worse, this is independent of these multi-project repo examples. If you simply open your editor in one of the src directories, then you wouldn't look at the sibling node_modules folder either.

@andrewbranch
Copy link
Member

Yeah. Such a setting could never ever ever be inferred from where you open your editor. It would have to be a compiler option. But I think there are legitimate use cases outside of performance. If you’re planning to ship a package to npm, or collaborate on a repo with other people, or mount a codebase in a Docker image, you really don’t want it to work only because you accidentally had a node_modules folder with a bunch of @types, or worse, implementation packages, in an ancestor way above the repo root. If that happens, you’d really prefer TypeScript to warn you about it, so you’re not shrugging at your coworkers saying “works on my machine 🤷‍♂️.”

@tusharf5
Copy link

tusharf5 commented Apr 9, 2024

We have similar use case where we want to publish some modules from the project_root/sdk path that has its own project_root/sdk/package.json and a lot of times we miss adding a dependency to the project_root/sdk/package.json because the dependency gets resolved from project_root/package.json so typescript doesn't complain.

We'd like the feature to be able to stop the package lookups from outside the project_root/sdk/tsconfig.json directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants