Proposal: importModuleSpecifierPreference: project-relative

[andrewbranch]

Fixes #36624

People often want to use paths aliases to refer to “external” code (cross-project/package/some other conceptual boundary), but use relative paths elsewhere. Auto-imports cannot help you do this today.

The problem is that the conceptual boundaries within which people want to use relative imports and across which they want to use paths aliases can’t always be inferred. Suppose I have a monorepo structure with a base tsconfig defining paths like

"paths": {
  "*": "./packages/*"
}

Because we are familiar with common monorepo patterns, it’s clear that the package boundaries are the direct subdirectories of packages. So within ./packages/foo I’d use relative imports to import anything in that package, and "bar" to import ./packages/bar.

However, it’s also common for webpack-bundled apps to have a convenience alias for the project root:

"paths": {
  "@/*": "./src/*"
}

This is structurally identical, but serves a different purpose—I needn’t treat ./src/utils and ./src/routes as separate, siloed directory trees.

My original hope was to be able to split module specifier suggestion behavior based on how paths is defined, but I think that is too hand-wavy. Instead, this PR introduces two more conservative heuristics:

1. If the path from an importing file to the imported file goes outside the directory of the importing file’s tsconfig.json (or inferred/unconfigured project root) and a non-relative path is available, use it.
2. If the nearest package.json file to the importing file and the nearest package.json file to the imported file are not the same file, and a non-relative path is available, use it.

For everything else, use a relative path.

This should cover basically all monorepo / project references configurations. I can imagine scenarios where I want to conceptually silo directories within a single project where these heuristics wouldn’t help you, but I haven’t heard user feedback from anyone with a setup like that.

I am proposing renaming importModuleSpecifierPreference: 'auto' to 'shortest' to be more descriptive, leaving it as the default (which means that clients sending 'auto' will continue to work), and giving this new setting a brief but descriptive name. Right now it’s 'external-non-relative', which is terrible.

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

[AlCalzone]

@andrewbranch should the improved behavior for same-package-imports already be present in 4.1-beta or is this PR required?
Currently, I'm getting this

instead of auto-importing ./SerialAPICommandMachine (with typescript.preferences.importModuleSpecifier: auto).

[andrewbranch]

@AlCalzone that’s probably a case of #40713

[RyanCavanaugh]

Kind of nervous that auto is going away; can you explain?

[andrewbranch]

I’ve renamed auto to shortest to be more descriptive. Previously, auto was an acceptable name because it would pick between the other options (relative and non-relative) based on some heuristic (number of path separators). Now, there are two options that pick between relative and non-relative based on some heuristic. auto becomes a bad name because it no longer encompasses all of the other options, it’s no more or less automatic than the new mode, and its name conveys nothing about the heuristic it uses. Values of "auto" will still map to "shortest", by merit of it being the default.

[RyanCavanaugh]

💯

[RyanCavanaugh]

Let's bikeshed this name somewhere

[andrewbranch]

Renamed to “project-relative.” How do we feel about this going into 4.1?

[andrewbranch]

Since this hasn’t gotten much attention and we’re now quite close to the RC, I think I’d prefer to wait for 4.2 for this. To get user feedback on it, I might see if there’s any interest in running a VS Code Insiders experiment setting project-relative as the default.

[AlCalzone]

How can one participate in that experiment?

[andrewbranch]

@typescript-bot pack this

You can try it out now by installing the tarball that @typescript-bot will post shortly, and adding to .vscode/settings.json:

{
  "typescript.tsserver.importModuleSpecifierPreference": "project-relative"
}

After this is properly merged, I’ll add a corresponding option to VS Code’s preferences UI.

[typescript-bot]

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

[typescript-bot]

Hey @andrewbranch, 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/87812/artifacts?artifactName=tgz&fileId=55909EDEAB61EA6ACF101E1289AB6877AC9DFF70C10B7C9D71CEB5027B30DFAA02&fileName=/typescript-4.1.0-insiders.20201027.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[AlCalzone]

@andrewbranch Do I need to configure project references, paths or the project-specific tsconfig.json in some specific way for this to work? My first test with "project-relative" just yielded an import that is relative to the root tsconfig's baseUrl (like it did before without this PR).

If you want to try:

1. clone https://github.com/zwave-js/node-zwave-js/tree/project-relative
2. open packages\zwave-js\src\lib\driver\Driver.ts
3. Line 1650 will have an error -> trigger quick fixes
4. 
   I would have expected this to be a relative import (../commandclass/DeviceResetLocallyCC)

[andrewbranch]

Oops, apparently the TypeScript language support extension in VS Code filters out values it doesn’t recognize. I guess I’ll have to make an update there first.

[AlCalzone]

@andrewbranch please let me know when there's something I can test. Not sure how long the process to update the extension takes.

[andrewbranch]

@AlCalzone you can use the TypeScript nightly and VS Code Insiders now:

[AlCalzone]

@andrewbranch I must be missing a puzzle piece...
I've updated the branch I mentioned above, current VSCode insiders, today's TS nightly. The setting in .vscode/settings.json is no longer greyed out:

but the repro above is unchanged:

Restarted TSServer and VSCode, no dice.

[andrewbranch]

Ah, it’s because you have this package symlinked through node_modules... and node_modules dependencies get priority. I didn’t notice that before. I’ll have a separate fix up for that.

[andrewbranch]

Wait, this is only allowed to happen because zwave-js is in the dependencies of the root package.json. I guess you could reasonably argue it’s still a bug for any package to import from itself through node_modules, but the reason I’ve never seen this before is because normally packages cannot look up at a package.json and see themselves listed as a dependency. This is also the first time I’ve noticed that you’re using lerna—lerna tends to work perfectly with project references without paths at all, and also without these file: dependencies in package.json files. But one thing at a time, what are these dependencies for in the root package.json?

[AlCalzone]

But one thing at a time, what are these dependencies for in the root package.json?

lerna adds them when I run lerna bootstrap. Apparently this happens because I use lerna link convert. That command is not really documented, but symlinks the local packages together, hoists all devDependencies into the root package.json and uses these file: dependencies.

See also this discussion:

If you add new packages be sure to add them to the dependencies list in the root package.json (you'll notice lerna link convert has done it for you for the ones you already have).

---

lerna tends to work perfectly with project references without paths at all

That would be great news, I've always thought TypeScript needs paths to understand the prefixed packages before they are actually being compiled.

[AlCalzone]

@andrewbranch FYI I just tried to migrate the project to npm@7 workspaces. Without the root package dependences, without paths and without baseUrl it works like a charm:
https://github.com/zwave-js/node-zwave-js/tree/04fee46ad6681574ff0280155a2d2b0d85588750
==>

So I guess if you don't want to support lerna's quirks, this is a documentation issue in the end.

[andrewbranch]

Huh, I guess I haven’t been using lerna link convert. lerna bootstrap already handles symlinking, and I already install common devDependencies in the root. Thanks for the tip... one more monorepo configuration edge case to consider.

[shrinktofit]

@andrewbranch How can I prevent import specifier suggestion from suggesting non-entry modules in other "packages(folder)" in same repo?
For example I have:

libs
  lib-a/
    index.ts
    internal-a.ts
  lib-b/
    index.ts
    interal-b.ts

I hope the specifier suggestion only suggest exports of lib-a/index.ts when triggered in lib-b/*.

[andrewbranch]

Exports available from the package index have the highest priority, but you can’t prevent exports in other files from showing up.