Expose importModuleSpecifierEnding to protocol

[andrewbranch]

Fixes #24779
VS Code PR: microsoft/vscode#90405
Pre-existing test (the feature is old)

[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

[jessetrinity]

Our current behavior seems to be that of the "index" setting. In the vscode PR the default is set to "minimal." Is that a more common usage?

[andrewbranch]

@jessetrinity good catch, the default in TS Server depends on the project’s module/moduleResolution settings. I guess we need an “auto” setting to pass undefined so TS Server can continue to pick the best one based on project.

For that matter, shouldn’t we just pick .js automatically when module is es2015+? I’m not a module system expert but I don’t know why you wouldn’t want that... paging @weswigham for thoughts?

[andrewbranch]

@jessetrinity updated the VS Code PR with an “auto” setting that leaves the existing default-picking intact. Thanks! I still want to look at selecting "js" by default in some cases before merging here.

[weswigham]

For that matter, shouldn’t we just pick .js automatically when module is es2015+? I’m not a module system expert but I don’t know why you wouldn’t want that...

That is a loaded question. es2015 doesn't always mean es2015. So, if your intended final runtime is browser-based es modules (or the still experimental node ones), then yes, you unconditionally want the extensions. If your final target is cjs; then probably not. Thing is, you might be checking with typescript, but the compiling with babel or webpack, and so as far as TS knows, you're targeting es2015 modules, but your runtime resolver is that of commonjs.

In reality, our module compiler flag is confusing two things that really need to be disentangled:

1. What version of which resolver are you targeting, which should be independent from by defaulted by
2. What runtime module system are you targeting

Really we should probably have more than two moduleResolution flags; the node flag is currently used as a catchall and might change behavior based on the module flag when it should/should not (we don't know).

[andrewbranch]

I think there’s a subtlety I’m missing about resolver differences—if you’re checking with TS and compiling with Babel, why wouldn’t you still just use --module=commonjs during checking?

[weswigham]

@andrewbranch only if you're not using TS to strip the type annotations and you're using the babel plugin. Before the babel plugin existed, you'd go thru TS to strip annotations (with target: esnext) and then thru babel (a two step pipeline). So projects with that kind of setup may still exist.

[andrewbranch]

Hmm, well that's kind of unfortunate, but still, I don’t think it would be breaking per se to use that as a heuristic for auto-import endings:

• CommonJS allows .js endings, so the code we generate works
• We let you write either, in any module mode, AFAIK, so you don’t get errors
• Users who want to keep their weird project settings but keep their minimal auto-imports can do so via the VS Code setting introduced by this PR’s counterpart

So is the heuristic good enough to be worth it? Is it going to be correct for most people?

[weswigham]

Probablyish We're going to have to deal with the commonjs vs esm resolver distinction soon enough anyway, though; node's export maps will stabilize eventually, and support conditionally returning different paths to each resolver (despite my protest); so knowing which the code is targeting is super important.

[andrewbranch]

I’m starting to dislike this as an editor preference because it feels like if you enable .js, you probably need that on all your extensions and we should be able to enforce that by a tsconfig setting (#28288). But it’s unclear to me what the option should be, how it will interact with existing options and upcoming ESM work, and whether an acceptable solution that would cover #24779, which is labeled VS Code Priority, is on the table for 3.9.

[DanielRosenwasser]

Yes, when we discussed this offline there was a lot of ambiguity including

• how do you resolve main fields? Do you import relative to the current module through node_modules, or do you keep rolling with Node's resolution?
• what do you do with .jsx paths? (presumably you never append .jsx)

Really the only people who benefit are people who have no dependencies

[andrewbranch]

We decided to move forward with this in the design meeting—any final feedback or concerns?

[jessetrinity]

Looks good to me with the auto setting.