Override `isExternalLibraryImport` as needed; re-add `realpath` #970

cspotcode · 2020-03-02T16:27:26Z

The various realpath issues we've seen don't strictly relate to symlinks.

They're caused by TypeScript considering a file to be an "external library import" when it's nested within a node_modules directory, and refusing to emit .js for it. Sometimes a symlink's path does not include node_modules and its resolved path does, or vice versa, so enabling or disabling realpath causes or fixes this bug.

The "external library import" determination is handled by the module resolver. If we implement our own in the LanguageServiceHost, we can tell the compiler to consider all rootFileNames to be "internal" no matter what.

This lets us re-enable realpath.

Related / should fix:
fix #876
fix #797
fix #956
fix #819
fix #990
fix #1093

Notes for code review

It's been a while since I started this PR, and I've since learned more about the compiler.

rootFiles are always emitted, but we want to avoid modifying rootFiles as much as possible, because it throws away a bunch of compiler state. How much, precisely, I'm not sure, but modify rootFiles is way slower than leaving it as-is rootFiles.

This PR hooks into TypeScript's resolver and does 2x things:
(a) forces some modules to be classified internal using our own logic, overriding TypeScript's logic
(b) keeps track of modules that have already been classified as internal

(a) enables compiling files nested within ./node_modules
(b) allows us to avoid modifying rootFiles unnecessary

TS files in node_modules are always forced to be internal, because I can't see any downsides to this.
JS files in node_modules are only forced internal on-demand (when required), because the "internal/external" distinction affects maxNodeModuleJsDepth, which affects typechecking.

Old, irrelevant notes

I hate deleting old research in case I need it later, but stuff below this line is not relevant anymore.

Gotchas / notes

(this is not true anymore) Implementation overrides isExternalLibraryImport according to our ignored() logic, which is the "nuclear option" described here:
microsoft/TypeScript#28432 (comment)
There's another option: node_modules nested within the rootDir are excluded, but if rootDir has node_modules in its own path, it is still compiled.

Implement resolveModuleNames that forces modules to be considered internal (not from an external library) when we need them to be emitted

coveralls · 2020-03-02T16:31:04Z

Coverage increased (+1.05%) to 86.95% when pulling cc279f8 on ab/realpath-and-emit-node_modules-files into 836d1f2 on master.

…t-node_modules-files

arcanis · 2020-04-19T14:21:48Z

Of note, TS has itself a bug with workspaces being marked as external sources (cf microsoft/TypeScript#37270). Maybe the isExternalLibraryImport override here could workaround that as well?

…t-node_modules-files

…ll require()d files to force them to be included in compilation without modifying the rootFiles array

codecov · 2020-05-20T02:13:10Z

Codecov Report

Merging #970 into master will decrease coverage by 0.31%.
The diff coverage is 82.25%.

@@            Coverage Diff             @@
##           master     #970      +/-   ##
==========================================
- Coverage   79.63%   79.32%   -0.32%     
==========================================
  Files           7        7              
  Lines         658      711      +53     
  Branches      147      158      +11     
==========================================
+ Hits          524      564      +40     
- Misses         84       90       +6     
- Partials       50       57       +7

Flag	Coverage Δ
#node_10	`76.28% <82.25%> (-0.09%)`	⬇️
#node_12_15	`76.63% <82.25%> (-0.12%)`	⬇️
#node_12_16	`76.63% <82.25%> (-0.12%)`	⬇️
#node_13	`79.04% <82.25%> (-0.29%)`	⬇️
#node_14	`79.04% <82.25%> (-0.29%)`	⬇️
#typescript_2_7	`78.90% <82.25%> (-0.28%)`	⬇️
#typescript_latest	`78.19% <82.25%> (-0.22%)`	⬇️
#typescript_next	`78.05% <82.25%> (-0.21%)`	⬇️
#ubuntu	`79.04% <82.25%> (-0.29%)`	⬇️
#windows	`79.18% <82.25%> (-0.30%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/index.ts	`79.06% <82.25%> (-0.54%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 836d1f2...cc279f8. Read the comment docs.

…t-node_modules-files

cspotcode · 2020-07-30T04:47:21Z

https://github.com/TypeStrong/ts-node/blob/master/src/index.ts#L688-L710

I noticed some weirdness in the compilerHost codepath which I'll need to fix. Don't have time now, so I'm jotting it down.

The bug is: rootFileNames are updated and the program rebuilt only when the builder program doesn't already have a sourcefile for the require()d module.

The problem is that a sourcefile might exist, but it might be considered "external." When this happens, typescript refuses to emit .js for it. We can force TS to emit .js by adding the file to rootFileNames. Unfortunately this is slow because it triggers a program rebuild that should not otherwise be necessary.

If we mark all non-ignored files as "internal", this problem goes away. Alternatively, we keep track of which files are classified "external" and we add them to rootFileNames only when we must force a reclassification "tointernal."

cspotcode · 2020-07-30T04:53:29Z

Importantly, we must ensure that tests properly trigger all possible patterns for a file being classified external, being omitted from rootFileNames, being unknown to the compiler, etc.

require() outside the typechecker
import from node_modules
something in node_modules import something else in node_modules (the latter is classified external b/c is not in rootFileNames)

cspotcode · 2020-08-11T17:35:21Z

development-docs/isExternalLibraryImport.md

@@ -0,0 +1,63 @@
+## How we override isExternalLibraryImport


This file is notes to myself and any other ts-node developers. Given the audience, it's not as polished as the rest of ts-node, and I can remove it if needed.

…t-node_modules-files

cspotcode · 2020-08-11T20:04:53Z

@blakeembrey this is finally(!!) done, tested, and ready for a review. I added my own review comments to hopefully make the changes clearer.

cspotcode · 2020-08-15T18:57:13Z

@blakeembrey Despite the red X, tests are passing. The only failure is coverage, and I think that's only because compiler docs ask us to implement a function that the compiler never actually calls.

If you think you'll be busy for the next week, and if this sounds ok to you, I was thinking of asking for a code review on the TypeScript Discord. Compiler experts hang out there, so I think I'd be able to find a reviewer with the right skillset.

Gerrit0 · 2020-08-15T22:46:51Z

src/index.ts

@@ -515,6 +527,102 @@ export function create (rawOptions: CreateOptions = {}): Register {
  let getOutput: (code: string, fileName: string) => SourceOutput
  let getTypeInfo: (_code: string, _fileName: string, _position: number) => TypeInfo

+  const getCanonicalFileName = (ts as unknown as TSInternal).createGetCanonicalFileName(ts.sys.useCaseSensitiveFileNames)


This seems dangerous, shouldn't case sensitivity be determined based on the OS, or potentially the forceConsistentCasingInFileNames option?

Fair question. ts.sys.useCaseSensitiveFileNames is OS-dependent: true on Posix, false on Windows. As far as I can tell, createGetCanonicalFileName is how the compiler internally converts filenames to cache keys.

As far as I know, forceConsistentCasingInFileNames generates additional semantic diagnostics but doesn't affect resolver behavior. Even when forceConsistentCasingInFileNames is turned off, FOO.ts and foo.ts still resolve to the same file on Windows, right?

Ah, that makes sense. I was reading useCaseSensitiveFileNames as if there was another useCaseInsensitiveFileNames option, which it makes sense that there isn't. This should be fine then.

blakeembrey · 2020-08-16T22:30:36Z

If you think you'll be busy for the next week, and if this sounds ok to you, I was thinking of asking for a code review on the TypeScript Discord.

Absolutely feel free to do this anyway!

blakeembrey

To be honest, I'll just trust you on this release 😄

This work is really great, it'll probably take me a while to verify the investigation you've already done!

blakeembrey · 2020-08-16T22:33:55Z

src/index.ts

 import sourceMapSupport = require('source-map-support')
 import * as ynModule from 'yn'
 import { BaseError } from 'make-error'
 import * as util from 'util'
 import { fileURLToPath } from 'url'
-import * as _ts from 'typescript'
+import type * as _ts from 'typescript'


blakeembrey · 2020-08-16T22:35:36Z

src/index.ts

+
+    // Get bucket for a source filename.  Bucket is the containing `./node_modules/*/` directory
+    // For '/project/node_modules/foo/node_modules/bar/lib/index.js' bucket is '/project/node_modules/foo/node_modules/bar/'
+    const moduleBucketRe = /.*\/node_modules\/[^\/]+\//


What about namespaced @ node modules?

Ooh, good catch. I'll add logic for this. I should also add a test with 2x peer modules, @org/foo and @org/bar, and make sure one bucket doesn't affect the other.

…t-node_modules-files

cspotcode added 2 commits March 2, 2020 11:18

Re-add "realpath" to LanguageServiceHost

42f13b0

Implement resolveModuleNames that forces modules to be considered internal (not from an external library) when we need them to be emitted

fix linter failures

99cfd15

fix failure on old ts version

18c1c54

cspotcode added this to In Progress in Andrew Bradley's Tasks Mar 2, 2020

Merge remote-tracking branch 'origin/master' into ab/realpath-and-emi…

c9addde

…t-node_modules-files

cspotcode mentioned this pull request Mar 26, 2020

failed to import TS file from node_modules on Linux & Mac #990

Closed

This was referenced Apr 7, 2020

projectVersion not bumped when rootFileNames changes #996

Closed

[WIP] combining coupled PRs into one to see if tests pass #1003

Closed

cspotcode marked this pull request as draft April 13, 2020 21:47

cspotcode mentioned this pull request May 1, 2020

TypeScript, Ts-Node, isExternalLibraryImport #1021

Closed

cspotcode added 2 commits May 19, 2020 20:41

Merge remote-tracking branch 'origin/master' into ab/realpath-and-emi…

d869941

…t-node_modules-files

Add $$ts-node-root.ts synthetic root file, which /// <reference's a…

84f7ccb

…ll require()d files to force them to be included in compilation without modifying the rootFiles array

Preserve path casing in /// <references

cfc8ce7

cspotcode mentioned this pull request Jun 3, 2020

ts-node treats symbolic links in node_modules as separate files #1064

Closed

cspotcode added 2 commits July 28, 2020 21:22

fix

18b517f

Revert $$ts-node-root changes, limiting only to the isExternal* changes

32d7c37

cspotcode force-pushed the ab/realpath-and-emit-node_modules-files branch from 83518b8 to 32d7c37 Compare July 29, 2020 01:40

cspotcode added 2 commits July 28, 2020 23:27

Add new resolver behavior to the CompilerHost codepath

7faf2fb

Merge remote-tracking branch 'origin/master' into ab/realpath-and-emi…

3ae44c6

…t-node_modules-files

This was referenced Aug 3, 2020

Avoid rootFiles mutation via synthetic $$ts-node-rootfiles.ts script #1092

Open

Compilation is unbelievably slow #754

Closed

cspotcode added 3 commits August 10, 2020 16:16

WIP

4c3c190

add tests; plus fixes

fd1ecf6

Fix tests

21f719f

cspotcode commented Aug 11, 2020

View reviewed changes

cspotcode added 2 commits August 11, 2020 13:52

Code-reviewing myself

8e99da8

Merge remote-tracking branch 'origin/master' into ab/realpath-and-emi…

6740e12

…t-node_modules-files

cspotcode marked this pull request as ready for review August 11, 2020 17:55

cspotcode added 8 commits August 11, 2020 14:07

fix tests

fc197e7

add missing test files

56146fd

fix tests

3054bc4

fix tests

e67334b

fix linter

678d080

fix tests on windows

4b45141

remove comma from the diff

5eda60f

Merge remote-tracking branch 'origin/master' into ab/realpath-and-emi…

93ec124

…t-node_modules-files

cspotcode requested a review from blakeembrey August 11, 2020 20:05

This was referenced Aug 12, 2020

[WIP] support for project references in languageService codepath #1111

Draft

Added a transpile-only ESM loader (#1101) #1102

Merged

Different compilation results from tsc #1093

Closed

Update package.json

69a7ec0

Gerrit0 reviewed Aug 15, 2020

View reviewed changes

blakeembrey reviewed Aug 16, 2020

View reviewed changes

adding handling of @Scoped modules in bucketing logic; adds tests

1d80028

cspotcode force-pushed the ab/realpath-and-emit-node_modules-files branch from bbfe788 to 1d80028 Compare August 21, 2020 04:08

Merge remote-tracking branch 'origin/master' into ab/realpath-and-emi…

cc279f8

…t-node_modules-files

cspotcode merged commit c2a2048 into master Aug 21, 2020

This was referenced Aug 21, 2020

release v9.0.0 #1118

Closed

Ts-node fails when using symlinks #396

Closed

Does not support local file paths in package.json #855

Closed

cspotcode moved this from In Progress to Merged to master in Andrew Bradley's Tasks Oct 14, 2020

cspotcode deleted the ab/realpath-and-emit-node_modules-files branch January 18, 2021 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override `isExternalLibraryImport` as needed; re-add `realpath` #970

Override `isExternalLibraryImport` as needed; re-add `realpath` #970

cspotcode commented Mar 2, 2020 •

edited

coveralls commented Mar 2, 2020 •

edited

arcanis commented Apr 19, 2020

codecov bot commented May 20, 2020 •

edited

cspotcode commented Jul 30, 2020 •

edited

cspotcode commented Jul 30, 2020

cspotcode Aug 11, 2020 •

edited

cspotcode commented Aug 11, 2020

cspotcode commented Aug 15, 2020

Gerrit0 Aug 15, 2020

cspotcode Aug 15, 2020

cspotcode Aug 15, 2020

Gerrit0 Aug 15, 2020

blakeembrey commented Aug 16, 2020

blakeembrey left a comment

blakeembrey Aug 16, 2020

blakeembrey Aug 16, 2020

cspotcode Aug 17, 2020

Override isExternalLibraryImport as needed; re-add realpath #970

Override isExternalLibraryImport as needed; re-add realpath #970

Conversation

cspotcode commented Mar 2, 2020 • edited

Notes for code review

Old, irrelevant notes

Gotchas / notes

coveralls commented Mar 2, 2020 • edited

arcanis commented Apr 19, 2020

codecov bot commented May 20, 2020 • edited

Codecov Report

cspotcode commented Jul 30, 2020 • edited

cspotcode commented Jul 30, 2020

cspotcode Aug 11, 2020 • edited

Choose a reason for hiding this comment

cspotcode commented Aug 11, 2020

cspotcode commented Aug 15, 2020

Gerrit0 Aug 15, 2020

Choose a reason for hiding this comment

cspotcode Aug 15, 2020

Choose a reason for hiding this comment

cspotcode Aug 15, 2020

Choose a reason for hiding this comment

Gerrit0 Aug 15, 2020

Choose a reason for hiding this comment

blakeembrey commented Aug 16, 2020

blakeembrey left a comment

Choose a reason for hiding this comment

blakeembrey Aug 16, 2020

Choose a reason for hiding this comment

blakeembrey Aug 16, 2020

Choose a reason for hiding this comment

cspotcode Aug 17, 2020

Choose a reason for hiding this comment

Override `isExternalLibraryImport` as needed; re-add `realpath` #970

Override `isExternalLibraryImport` as needed; re-add `realpath` #970

cspotcode commented Mar 2, 2020 •

edited

coveralls commented Mar 2, 2020 •

edited

codecov bot commented May 20, 2020 •

edited

cspotcode commented Jul 30, 2020 •

edited

cspotcode Aug 11, 2020 •

edited