Proposal: If there’s a package.json, only auto-import things in it, more or less by andrewbranch · Pull Request #31893 · microsoft/TypeScript (original) (raw)

This started out as an attempt to solve #30713 in the general case, as an alternative to #31065. As it stands, it’s possible that this doesn’t change the status of #30713 at all, depending on the specifics of the project, but my hunch is that this is, in other ways, an improvement over the status quo.

You shouldn’t import node_modules that aren’t in your package.json

Your node_modules folder has bazillions of things in it as dependencies of dependencies that you shouldn’t import directly without explicitly adding them to your own top-level dependencies (in your package.json). And yet, if any of these second-order dependencies have typings (whether included or in @types), TypeScript happily offers to import it for you. This is especially frustrating when the correct import would be offered to you were it not shadowed by a wrong one. For example, I installed @emotion/core which re-exports some symbols from @emotion/css. Obviously I want to import from my explicit dependency of @emotion/core, not one of its dependencies:

Before (GIF)

Imports from @emotion/css because that’s the shorter path. ESLint rule no-extraneous-dependencies enabled for emphasis.

After (GIF)

Imports from @emotion/core because that’s an explicit dependency in my package.json.

Special handling for Node core modules

Core modules like os and crypto are the one exception to the rule. Actually, in a TypeScript project, they’re almost not an exception, because the user is expected to have @types/node in their package.json if they’re using those modules. But in a JavaScript project supported by type acquisition in VS Code, the user will probably not explicitly install @types/node, but will still expect IntelliSense and auto-import to work for those modules if they’re writing for Node. If they’re purely writing for the browser, they expect not to see auto-import suggestions for them (#30713).

In a JavaScript project, especially one that tends not to install @types and doesn’t have a jsconfig.json, I haven’t found a bulletproof heuristic for whether or not to offer Node core module imports. Today, they’re offered if you have @types/node in your program for any reason, including as a dependency of a dependency, which happens pretty frequently.

My proposed heuristic is “are you already importing Node core modules.” If you are, we should probably help you import more of them. If you’re not, either you don’t want to import any, or we’ll help you after you write the first one by hand. We can also define this more or less conservatively, depending on where we think is the best balance between minimizing false positives, minimizing false negatives, and maximizing performance:

Does the file you’re editing import core modules? Super cheap to check, and if yes, we can definitely offer the auto-import. (PR currently does this.)
Do any other script files in the same directory as the file you’re editing import core modules? A little more expensive to check, but eliminates a lot of false negatives (not offering an import when we should), with low chance of introducing false positives (offering an import when we shouldn’t). (This might be the best middle ground IMO.)
Do any other script files in your program import core modules? More expensive to check, but likely not outrageously so, and could be cached. Likely introduces false positives for many projects that are primarily browser apps with a small amount of Node build scripts. (I don’t think this is a great option.) JS auto-import from node core heuristic (GIF)

writeFile is offered as an auto-import once there’s another import from a Node core module in the file.

Open questions

Performance and code reuse. There are a few different places that analyze package.json files in the language service now (string literal completions for module specifiers is the other most significant). I initially tried to pull some functions from other places into some common utils, but the other use cases were just different enough that it was non-trivial to generalize without sacrificing performance. Rather than invest the time into making a general util for getting info out of package.json(s) that runs every time someone is requesting it, I wondered if it would be worthwhile to cache some of this in the language service so it can be reused for as long as the package.json stays the same, which is obviously much longer than script files on average. Thoughts?
Corner cases where this implementation will break. This PR seems to work in the tests I’ve written and playing around with small projects in VS Code, but I expect there will be feedback on how I’ve approached a couple things; I may have missed some subtle complexities. I’ll leave a review of my own code inline to point out some things I wasn’t 100% confident on.
Corner cases on why we might not want this at all? I think my logic is sound as far as the desirability of this feature goes, and I think the only place where we still might do the wrong thing is with JS and Node core modules, but we already do the wrong thing there some of the time. Projects almost always have a package.json from the first moment of their existence, but if they don’t we simply fall back to the current behavior of not filtering. But are there any other reasons why this might not be a good idea? Can you think of any edge cases where it’s deal-breakingly disruptive?