external modules resolve by node_modules and package.json-main

[basarat]

Code for #2338. This is just for the node_modules resolution. Does everything for node_modules except for the typings stuff as I don't see the rational for that considering there is no way to have tsc send .js to one location and .d.ts to another location.

[](No longer relevant :
using ts.sys.fileExists doesn't work under the test harness because it doesn't give actual file system paths. So tests are broken by this commit. What is the best way to overcome this. I have reviewed [previous work]%28master...Nemo157:node_modules) that uses program.getSourceFile %28which resolves to [this code]%28https://github.com/Microsoft/TypeScript/blob/e76439b2ff39e1e8afd926450e7cebba2149fe28/src/harness/projectsRunner.ts#L212-L222%29%29 to do exists checks and read package.json and that feels very ugly. How would you guys recommend authoring the resolveExternalModule function.)

[DanielRosenwasser]

Omit the type annotation

[basarat]

Done

[basarat]

How would you guys recommend authoring the resolveExternalModule function.

I've added a commit which gets the tests passing to demonstrate the issue : a781916

[DanielRosenwasser]

Thanks for the PR @basarat - I got the chance to discuss it a bit with @vladima and apparently there are some subtleties on our end, some of which pertain to language service scenarios. Vlad has some existing work on this, and may be able to explain a bit more.

[basarat]

apparently there are some subtleties on our end, some of which pertain to language service scenarios

If it is because the language service didn't get such a resolved file as a part of its original file set : then this can be solved by lazily expanding the file set : https://github.com/TypeStrong/atom-typescript/blob/8df1482222fb88363b504d9c27f98692e938e44f/lib/main/lang/core/languageServiceHost2.ts#L353-L358 I've tried the code in this PR with atom-typescript and it works like a charm 🌹

[vladima]

I guess main points of inconvenience on our end are:

1. when language service is hosted inside the VS you in general don't have access to all sys functionality meaning that operations like isDirectoryExists, isFileExists and readFile should be delegated to the host.
2. the way how VS side works today is slightly different from the model used by tsserver. Main difference is that currently VS pre-resolves all tripleslash-refs/imported files before creating script language service so if resolution process will be just changed on the script side - this will break VS integration.
3. per External module resolution logic #2338 we would like to have ability to switch module resolution from node style (with folder walks) to requirejs (that will use baseURL) and vice versa. Ideally module resolver should just a function (moduleName: string, containingFile: string, compilerHost: CompilerHost) => string so if somebody needs more involved method to resolve modules - he can do this by just replacing this one part. It does not directly contradict with proposed solution - it just needs to be slightly reshaped.

[basarat]

I guess main points of inconvenience

@vladima Taking your points into consideration I guess you are recommending:

• adding readFile and fileExists to host ... and then using sys inside host.
• after that refactoring use the resolver function to be (moduleName: string, containingFile: string, compilerHost: CompilerHost) => string and use host.readFile/host.fileExists inside.

So:

• ^ Is the refactoring I am suggestion correct?
• What is shim host? < you might same me some time digging through the code :)

[vladima]

What is shim host?

do you mean this guy ? The idea behind it (and shims in general) is that they act as a send\receive proxies whose purpose is to extract incoming parameters from JSON, call corresponding method on the language service instance, wrap its return value to JSON and send it back.

Is the refactoring I am suggestion correct?

In general yes, you got the idea. I need to think more about details of interaction with VS that should be handled.

[basarat]

do you mean

Yup. Thanks. I'll see if it comes up in this refactoring (I am sure it will) :)

In general yes, you got the idea

I'll give it a go 🌹

[basarat]

Investigating a better method

Pushed basarat@a17c6aa

The current changeset is ready for an early review if anyone is interested. I'm moving on to adding tests for this.

[basarat]

Tests pushed. The PR is ready for review 👍 🌹

[MicahZoltu]

TSC may not be able to output a d.ts to a separate location, but a post-compile build step easily could. It seems to be fairly common for typings to sit in a typings directory.

That being said, I believe there is value in setting a standard for how people should distribute their TS/JS packages (no typings folder) and expand it later if necessary.

[basarat]

believe there is value in setting a standard for how people should distribute their TS/JS packages (no typings folder) and expand it later if necessary.

Thanks ❤️

Also at the moment the typings from the typings property are currently of the form declare module "foo/bar" and those typings are not the target of this PR anyways 🌹

[poelstra]

👍 on this functionality!

[vladima]

I've pulled with PR into the moduleResolution branch in this repo - this will cover node related part of #2338 and the rest of the work on this proposal will be continued in that branch.

Thanks!

[basarat]

Thanks @vladima!

[mhegazy]

This should be in the 1.6 release