Proposal: If there’s a package.json, only auto-import things in it, more or less #31893

andrewbranch · 2019-06-13T19:25:13Z

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?

src/services/codefixes/importFixes.ts

andrewbranch · 2019-06-13T19:35:31Z

src/services/codefixes/importFixes.ts

+        let seenDeps: Map<true> | undefined;
+        function *readPackageJsonDependencies() {
+            type PackageJson = Record<typeof dependencyKeys[number], Record<string, string> | undefined>;
+            const dependencyKeys = ["dependencies", "devDependencies", "optionalDependencies"] as const;


I saw string completions consider peerDependencies, but I was on the fence about whether that makes sense here. Usually a peer dependency is also a dev dependency; if it’s not, we probably don’t have typings for it anyway, and would never offer an auto-import in the first place. It seems unlikely to me that anyone will notice/care whether we look there.

Yeah, I think excluding peer deps is the right thing to do here, if it's not in your dependencies (or through other deps) then it won't be in the tree at all

andrewbranch · 2019-06-13T19:50:10Z

src/services/completions.ts

-                            // If `!!d.name.originalKeywordKind`, this is `export { _break as break };` -- skip this and prefer the keyword completion.
-                            // If `!!d.parent.parent.moduleSpecifier`, this is `export { foo } from "foo"` re-export, which creates a new symbol (thus isn't caught by the first check).
-                            isExportSpecifier(d) && (d.propertyName ? isIdentifierANonContextualKeyword(d.name) : !!d.parent.parent.moduleSpecifier))) {
+                    if (typeChecker.getMergedSymbol(symbol.parent!) !== resolvedModuleSymbol) {


I need to look more at this case and see if we can still always continue—I think we might need to do something similar to below, but forgot to investigate further.

Yep, I was right—fixed in de8ef32

src/services/completions.ts

andrewbranch · 2019-06-13T19:55:19Z

src/services/completions.ts

                    }
+                    else {
+                        // This is not a re-export, so see if we have any aliases pending and remove them
+                        potentialDuplicateSymbols.delete(getSymbolId(symbol).toString());


I was unable to come up with a test case where this happens. I think a well-behaved language service host probably tends to order source files in such a way that we always see the original symbol first and the re-exports later, but I think it’s possible that with circular imports or a less savvy language service host, we would need this branch.

RyanCavanaugh · 2019-06-13T20:09:03Z

cc @mjbvz

src/services/completions.ts

niieani · 2019-06-14T09:26:54Z

Not sure if this is covered (I didn't see it being mentioned), but please remember about monorepos, which are configured as a single project (i.e. single {js,ts}config in the root directory).

In all cases we should use the package.json closest to the file that is being edited, rather than the one closest to the project's root ({js,ts}config).

andrewbranch · 2019-06-14T15:55:34Z

Hey @niieani, thanks for the feedback! The behavior I’ve coded here is to look for all package.jsons in scope for the current file and offer auto-imports from all of them. You’re right that using the one closest to the project root would be insufficient, but using only the closest one I think would be too restrictive—for example, electron-react-typescript-boilerplate uses one package.json at the root for most dependencies, and a second one in app/ specifically for native dependencies that need to be built for multiple platforms when packaging.

(Once I hear some feedback from the team about whether we want to go forward with this, I’ll add a test case for this behavior too)

sheetalkamat · 2019-06-26T18:53:06Z

src/services/codefixes/importFixes.ts

@@ -620,4 +666,73 @@ namespace ts.codefix {
        // Need `|| "_"` to ensure result isn't empty.
        return !isStringANonContextualKeyword(res) ? res || "_" : `_${res}`;
    }
+
+    function createLazyPackageJsonDependencyReader(fromFile: SourceFile, host: LanguageServiceHost) {
+        const packageJsonPaths = findPackageJsons(getDirectoryPath(fromFile.fileName), host);


I think from tsserver perspective caching packageJson should be fairely ok since we do watch failed lookup locations and any changes in there result in recomputing program. But that's not guaranteed with other hosts since they can have their own module resolution or use the default resolution cache which doesn't watch these locations.

we do watch failed lookup locations and any changes in there result in recomputing program

Just to make sure I’ve interpreted this correctly: you’re saying that if a user runs npm install new-package which changes the package.json, synchronizeHostData will be called so I could update the cache there?

But that's not guaranteed with other hosts

Maybe we can implement a fast check to determine whether the cache is up-to-date (last modified timestamp? content hash?). Actually, that might be better than reading eagerly on recomputing the program, since the program will change more often than the package.json.

sandersn

I don't understand the overall structure of the code that's changed in completions.ts. Can you explain what alias-vs-not-alias is and exported-vs-reexported? I need to get a big picture before I can understand the specific uses of things like potentatialDuplicateSymbol and coveredReExportedSymbols.

Also a few style suggestions.

src/services/completions.ts

andrewbranch · 2019-06-26T23:40:44Z

Thanks for the review @sandersn. I added a much more cohesive comment with diagrams and examples. It may be worth mentioning separately, though, that most of the apparent complexity is just a performance optimization. The goal is simply “don’t add aliases to the list iff the symbol they’re aliasing is in the list,” but the trick is doing that in a single pass in an arbitrary module order, aggressively caching work the type checker has already done. If we didn’t care about time complexity or calling checker.resolveAlias() way more times than we need to, the algorithm would probably look much simpler.

andrewbranch · 2019-07-03T00:29:37Z

Ping @mjbvz—could I get someone from the VS Code side to give this idea a quick look? I would love to get this in before we cut the beta release. If that doesn’t happen I’ll shoot for typescript@experimental, but either way we’ll want to get VS Code’s feedback since most requests/issues in this area come via the Code repo. Thanks! 😄

mjbvz · 2019-07-03T03:45:34Z

@andrewbranch I think the idea makes a lot of sense but we will need to see how it works for some real world projects. If this PR is merged, we can ask users on VS Code 1.36+ to installed this extension and see if there are any issues reported

orta · 2019-07-03T17:48:17Z

src/services/completions.ts

+         *    occur for that symbol---that is, the original symbol is not in Bucket A, so we should include the alias. Move
+         *    everything from Bucket C to Bucket A.
+         */
+        function getSymbolsFromOtherSourceFileExports(/** Bucket A */ symbols: Symbol[], tokenText: string, target: ScriptTarget, host: LanguageServiceHost): void {


sandersn

I still need to re-read completions.ts, but here are some style nits for now.

sandersn

ok, here are the nits this time. I hope.

sandersn

HERE are my minor nits, which I worked so hard to reproduce!

src/services/codefixes/importFixes.ts

sandersn

One more question and some minor suggestions for completions.ts.

src/services/completions.ts

…in it, more or less (#31893)" This reverts commit 60a1b1d.

…in it, more or less (microsoft#31893)" This reverts commit 60a1b1d.

…in it, more or less (#31893)" (#32448) This reverts commit 60a1b1d.

Component commits: f6cb90a Revert "Proposal: If there’s a package.json, only auto-import things in it, more or less (microsoft#31893)" This reverts commit 60a1b1d.

…in it, more or less (#31893)" (#32448) This reverts commit 60a1b1d.

andrewbranch commented Jun 13, 2019

View reviewed changes

andrewbranch requested review from sheetalkamat, rbuckton, DanielRosenwasser, mjbvz, orta, RyanCavanaugh, sandersn and weswigham June 13, 2019 20:20

andrewbranch commented Jun 13, 2019

View reviewed changes

src/services/completions.ts Outdated Show resolved Hide resolved

andrewbranch mentioned this pull request Jun 24, 2019

Don’t show auto-import suggestion if there’s a global by the same name #31065

Closed

sheetalkamat reviewed Jun 26, 2019

View reviewed changes

sandersn requested changes Jun 26, 2019

View reviewed changes

src/services/completions.ts Outdated Show resolved Hide resolved

src/services/completions.ts Outdated Show resolved Hide resolved

src/services/completions.ts Outdated Show resolved Hide resolved

andrewbranch requested a review from sandersn June 26, 2019 23:33

orta reviewed Jul 3, 2019

View reviewed changes

RyanCavanaugh assigned sheetalkamat Jul 8, 2019

sandersn requested changes Jul 10, 2019

View reviewed changes

sandersn self-assigned this Jul 10, 2019

sandersn requested changes Jul 10, 2019

View reviewed changes

sandersn requested changes Jul 11, 2019

View reviewed changes

andrewbranch added 3 commits July 11, 2019 15:04

Move package.json related utils to utilities

40321ad

Add failing test

b43f088

Make first test pass

7af4d37

andrewbranch added 13 commits July 11, 2019 15:04

Remove unused triple-slash ref

2d7eaf9

Update comment

306afe6

Refactor findAlias to forEachAlias to reduce iterations

488fa8f

Really fix re-exporting

98436c9

Use getModuleSpecifier instead of custom hack

cb2eef9

Fix offering auto imports to paths within node modules

7639059

Rename things and make comments better

d6767a8

Add another reexport test

7268bca

Inline symbolHasBeenSeen

3962d4a

Simplify forEachAlias to findAlias

b2ce461

Add note that symbols is mutated

f276466

Symbol order doesn’t matter here

ecd15e5

Style nits

ac0f70a

andrewbranch force-pushed the enhancement/only-import-from-package-json branch from cc3615c to ac0f70a Compare July 11, 2019 22:40

andrewbranch added 2 commits July 11, 2019 16:51

Add test with nested package.jsons

fbb05d5

Fix and add tests for export * re-exports

de8ef32

sandersn approved these changes Jul 12, 2019

View reviewed changes

andrewbranch merged commit 60a1b1d into microsoft:master Jul 12, 2019

andrewbranch deleted the enhancement/only-import-from-package-json branch July 12, 2019 17:09

IllusionMH mentioned this pull request Jul 17, 2019

Suggestions regression since 3.6.0-dev.20190713 #32441

Closed

andrewbranch added a commit that referenced this pull request Jul 17, 2019

Revert "Proposal: If there’s a package.json, only auto-import things …

dbefaf2

…in it, more or less (#31893)" This reverts commit 60a1b1d.

andrewbranch mentioned this pull request Jul 17, 2019

Revert "Proposal: If there’s a package.json, only auto-import things … #32444

Closed

andrewbranch added a commit to andrewbranch/TypeScript that referenced this pull request Jul 17, 2019

Revert "Proposal: If there’s a package.json, only auto-import things …

f6cb90a

…in it, more or less (microsoft#31893)" This reverts commit 60a1b1d.

andrewbranch mentioned this pull request Jul 17, 2019

Revert "Proposal: If there’s a package.json, only auto-import things in it" #32448

Merged

andrewbranch added a commit that referenced this pull request Jul 17, 2019

Revert "Proposal: If there’s a package.json, only auto-import things …

387c917

…in it, more or less (#31893)" (#32448) This reverts commit 60a1b1d.

DanielRosenwasser pushed a commit that referenced this pull request Jul 18, 2019

Revert "Proposal: If there’s a package.json, only auto-import things …

47cbe87

…in it, more or less (#31893)" (#32448) This reverts commit 60a1b1d.

andrewbranch mentioned this pull request Jul 22, 2019

Updated: Only auto-import from package.json #32517

Merged

andrewbranch mentioned this pull request Feb 4, 2020

clearTimeout is imported from timers #30713

Closed

DanielRosenwasser mentioned this pull request Apr 6, 2020

Expand auto-import to all dependencies in package.json #37812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: If there’s a package.json, only auto-import things in it, more or less #31893

Proposal: If there’s a package.json, only auto-import things in it, more or less #31893

andrewbranch commented Jun 13, 2019 •

edited

Loading

andrewbranch Jun 13, 2019 •

edited

Loading

orta Jun 13, 2019

andrewbranch Jun 13, 2019

andrewbranch Jul 12, 2019

andrewbranch Jun 13, 2019

RyanCavanaugh commented Jun 13, 2019

niieani commented Jun 14, 2019

andrewbranch commented Jun 14, 2019 •

edited

Loading

sheetalkamat Jun 26, 2019

andrewbranch Jun 26, 2019

sandersn left a comment •

edited

Loading

andrewbranch commented Jun 26, 2019

andrewbranch commented Jul 3, 2019

mjbvz commented Jul 3, 2019

orta Jul 3, 2019

sandersn left a comment

sandersn left a comment

sandersn left a comment

sandersn left a comment

Proposal: If there’s a package.json, only auto-import things in it, more or less #31893

Proposal: If there’s a package.json, only auto-import things in it, more or less #31893

Conversation

andrewbranch commented Jun 13, 2019 • edited Loading

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

Special handling for Node core modules

Open questions

andrewbranch Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

orta Jun 13, 2019

Choose a reason for hiding this comment

andrewbranch Jun 13, 2019

Choose a reason for hiding this comment

andrewbranch Jul 12, 2019

Choose a reason for hiding this comment

andrewbranch Jun 13, 2019

Choose a reason for hiding this comment

RyanCavanaugh commented Jun 13, 2019

niieani commented Jun 14, 2019

andrewbranch commented Jun 14, 2019 • edited Loading

sheetalkamat Jun 26, 2019

Choose a reason for hiding this comment

andrewbranch Jun 26, 2019

Choose a reason for hiding this comment

sandersn left a comment • edited Loading

Choose a reason for hiding this comment

andrewbranch commented Jun 26, 2019

andrewbranch commented Jul 3, 2019

mjbvz commented Jul 3, 2019

orta Jul 3, 2019

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

andrewbranch commented Jun 13, 2019 •

edited

Loading

andrewbranch Jun 13, 2019 •

edited

Loading

andrewbranch commented Jun 14, 2019 •

edited

Loading

sandersn left a comment •

edited

Loading