-
-
Notifications
You must be signed in to change notification settings - Fork 534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override isExternalLibraryImport
as needed; re-add realpath
#970
Changes from 14 commits
42f13b0
99cfd15
18c1c54
c9addde
d869941
84f7ccb
cfc8ce7
18b517f
32d7c37
7faf2fb
3ae44c6
4c3c190
fd1ecf6
21f719f
8e99da8
6740e12
fc197e7
56146fd
3054bc4
e67334b
678d080
4b45141
5eda60f
93ec124
69a7ec0
1d80028
cc279f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
## How we override isExternalLibraryImport | ||
|
||
`isExternalLibraryImport` is a boolean returned by node's module resolver that is `true` | ||
if the target module is inside a `node_modules` directory. | ||
|
||
This has 2x effects inside the compiler: | ||
a) compiler refuses to emit JS for external modules | ||
b) increments node_module depth +1, which affects `maxNodeModulesJsDepth` | ||
|
||
If someone `require()`s a file inside `node_modules`, we need to override this flag to overcome (a). | ||
|
||
### ts-node's behavior | ||
|
||
- If TS's normal resolution deems a file is external, we might override this flag. | ||
- Is file's containing module directory marked as "must be internal"? | ||
- if yes, override as "internal" | ||
- if no, track this flag, and leave it as "external" | ||
|
||
When you try to `require()` a file that's previously been deemed "external", we mark the entire module's | ||
directory as "must be internal" and add the file to `rootFiles` to trigger a re-resolve. | ||
|
||
When you try to `require()` a file that's totally unknown to the compiler, we have to add it to `rootFiles` | ||
to trigger a recompile. This is a separate issue. | ||
|
||
### Implementation notes | ||
|
||
In `updateMemoryCache`: | ||
- If file is not in rootFiles and is not known internal (either was never resolved or was resolved external) | ||
- mark module directory as "must be internal" | ||
- add file to rootFiles to either pull file into compilation or trigger re-resolve (will do both) | ||
|
||
TODO: WHAT IF WE MUST MARK FILEA INTERNAL; WILL FILEB AUTOMATICALLY GET THE SAME TREATMENT? | ||
|
||
TODO if `noResolve`, force adding to `rootFileNames`? | ||
|
||
TODO if `noResolve` are the resolvers called anyway? | ||
|
||
TODO eagerly classify .ts as internal, only use the "bucket" behavior for .js? | ||
- b/c externalModule and maxNodeModulesJsDepth only seems to affect typechecking of .js, not .ts | ||
|
||
### Tests | ||
|
||
require() .ts file where TS didn't know about it before | ||
require() .js file where TS didn't know about it before, w/allowJs | ||
import {} ./node_modules/*/.ts | ||
import {} ./node_modules/*/.js w/allowJs (initially external; will be switched to internal) | ||
import {} ./node_modules/*/.ts from another file within node_modules | ||
import {} ./node_modules/*/.js from another file within node_modules | ||
require() from ./node_modules when it is ignored; ensure is not forced internal and maxNodeModulesJsDepth is respected (type info does not change) | ||
|
||
### Keywords for searching TypeScript's source code | ||
|
||
These may jog my memory the next time I need to read TypeScript's source and remember how this works. | ||
|
||
currentNodeModulesDepth | ||
sourceFilesFoundSearchingNodeModules | ||
|
||
isExternalLibraryImport is used to increment currentNodeModulesDepth | ||
currentNodeModulesDepth is used to put things into sourceFilesFoundSearchingNodeModules | ||
|
||
https://github.com/microsoft/TypeScript/blob/ec338146166935069124572135119b57a3d2cd22/src/compiler/program.ts#L2384-L2398 | ||
|
||
getSourceFilesToEmit / sourceFileMayBeEmitted obeys internal "external" state, is responsible for preventing emit of external modules |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ before(async function () { | |
|
||
describe('ts-node', function () { | ||
const cmd = `"${BIN_PATH}" --project "${PROJECT}"` | ||
const cmdNoProject = `"${BIN_PATH}"` | ||
|
||
this.timeout(10000) | ||
|
||
|
@@ -545,20 +546,29 @@ describe('ts-node', function () { | |
return done() | ||
}) | ||
}) | ||
}) | ||
|
||
it('should give ts error for invalid node_modules', function (done) { | ||
exec(`${cmd} --compiler-host --skip-ignore tests/from-node-modules/from-node-modules`, function (err, stdout) { | ||
if (err === null) return done('Expected an error') | ||
|
||
expect(err.message).to.contain('Unable to compile file from external library') | ||
|
||
return done() | ||
}) | ||
it('should transpile files inside a node_modules directory when not ignored', function (done) { | ||
exec(`${cmdNoProject} --script-mode tests/from-node-modules/from-node-modules`, function (err, stdout, stderr) { | ||
if (err) return done(`Unexpected error: ${err}\nstdout:\n${stdout}\nstderr:\n${stderr}`) | ||
expect(stdout.trim()).to.equal(JSON.stringify({ | ||
external: { | ||
tsmri: { name: 'typescript-module-required-internally' }, | ||
jsmri: { name: 'javascript-module-required-internally' }, | ||
tsmii: { name: 'typescript-module-imported-internally' }, | ||
jsmii: { name: 'javascript-module-imported-internally' } | ||
}, | ||
tsmie: { name: 'typescript-module-imported-externally' }, | ||
jsmie: { name: 'javascript-module-imported-externally' }, | ||
tsmre: { name: 'typescript-module-required-externally' }, | ||
jsmre: { name: 'javascript-module-required-externally' } | ||
}, null, 2)) | ||
done() | ||
}) | ||
}) | ||
|
||
it('should transpile files inside a node_modules directory when not ignored', function (done) { | ||
exec(`${cmd} --skip-ignore tests/from-node-modules/from-node-modules`, function (err, stdout, stderr) { | ||
it('should respect maxNodeModulesJsDepth', function (done) { | ||
exec(`${cmdNoProject} --script-mode tests/maxnodemodulesjsdepth`, function (err, stdout, stderr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The magic happens in |
||
if (err) return done(`Unexpected error: ${err}\nstdout:\n${stdout}\nstderr:\n${stderr}`) | ||
done() | ||
}) | ||
|
@@ -567,17 +577,17 @@ describe('ts-node', function () { | |
|
||
describe('register', function () { | ||
let registered: tsNodeTypes.Register | ||
let moduleTestPath: string | ||
before(() => { | ||
registered = register({ | ||
project: PROJECT, | ||
compilerOptions: { | ||
jsx: 'preserve' | ||
} | ||
}) | ||
moduleTestPath = require.resolve('../tests/module') | ||
}) | ||
|
||
const moduleTestPath = require.resolve('../tests/module') | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Frankly I don't know how this was passing before this change. It should only resolve after we've registered ts-node. Maybe a race condition, where another test case already installed registered? |
||
afterEach(() => { | ||
// Re-enable project after every test. | ||
registered.enabled(true) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
import { relative, basename, extname, resolve, dirname, join } from 'path' | ||
import { relative, basename, extname, resolve, dirname, join, isAbsolute } from 'path' | ||
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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏 |
||
|
||
/** | ||
* Does this version of node obey the package.json "type" field | ||
|
@@ -94,6 +94,18 @@ export interface TSCommon { | |
formatDiagnosticsWithColorAndContext: typeof _ts.formatDiagnosticsWithColorAndContext | ||
} | ||
|
||
/** | ||
* Compiler APIs we use that are marked internal and not included in TypeScript's public API declarations | ||
*/ | ||
interface TSInternal { | ||
// https://github.com/microsoft/TypeScript/blob/4a34294908bed6701dcba2456ca7ac5eafe0ddff/src/compiler/core.ts#L1906-L1909 | ||
createGetCanonicalFileName (useCaseSensitiveFileNames: boolean): TSInternal.GetCanonicalFileName | ||
} | ||
namespace TSInternal { | ||
// https://github.com/microsoft/TypeScript/blob/4a34294908bed6701dcba2456ca7ac5eafe0ddff/src/compiler/core.ts#L1906 | ||
export type GetCanonicalFileName = (fileName: string) => string | ||
} | ||
|
||
/** | ||
* Export the current version. | ||
*/ | ||
|
@@ -498,6 +510,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems dangerous, shouldn't case sensitivity be determined based on the OS, or potentially the forceConsistentCasingInFileNames option? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I know, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that makes sense. I was reading |
||
|
||
// In a factory because these are shared across both CompilerHost and LanguageService codepaths | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO we should extract more logic into this style of "factory:" reusable bits of functionality that can be shared across our languageService and compilerHost codepaths. |
||
function createResolverFunctions (serviceHost: _ts.ModuleResolutionHost) { | ||
const moduleResolutionCache = ts.createModuleResolutionCache(cwd, getCanonicalFileName, config.options) | ||
const knownInternalFilenames = new Set<string>() | ||
/** "Buckets" (module directories) whose contents should be marked "internal" */ | ||
const internalBuckets = new Set<string>() | ||
|
||
// 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\/[^\/]+\// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about namespaced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
function getModuleBucket (filename: string) { | ||
const find = moduleBucketRe.exec(filename) | ||
if (find) return find[0] | ||
return '' | ||
} | ||
|
||
// Mark that this file and all siblings in its bucket should be "internal" | ||
function markBucketOfFilenameInternal (filename: string) { | ||
internalBuckets.add(getModuleBucket(filename)) | ||
} | ||
|
||
function isFileInInternalBucket (filename: string) { | ||
return internalBuckets.has(getModuleBucket(filename)) | ||
} | ||
|
||
function isFileKnownToBeInternal (filename: string) { | ||
return knownInternalFilenames.has(filename) | ||
} | ||
|
||
/** | ||
* If we need to emit JS for a file, force TS to consider it non-external | ||
*/ | ||
const fixupResolvedModule = (resolvedModule: _ts.ResolvedModule | _ts.ResolvedTypeReferenceDirective) => { | ||
const { resolvedFileName } = resolvedModule | ||
if (resolvedFileName === undefined) return | ||
// .ts is always switched to internal | ||
// .js is switched on-demand | ||
if ( | ||
resolvedModule.isExternalLibraryImport && ( | ||
(resolvedFileName.endsWith('.ts') && !resolvedFileName.endsWith('.d.ts')) || | ||
isFileKnownToBeInternal(resolvedFileName) || | ||
isFileInInternalBucket(resolvedFileName) | ||
) | ||
) { | ||
resolvedModule.isExternalLibraryImport = false | ||
} | ||
if (!resolvedModule.isExternalLibraryImport) { | ||
knownInternalFilenames.add(resolvedFileName) | ||
} | ||
} | ||
/* | ||
* NOTE: | ||
* Older ts versions do not pass `redirectedReference` nor `options`. | ||
* We must pass `redirectedReference` to newer ts versions, but cannot rely on `options`, hence the weird argument name | ||
*/ | ||
const resolveModuleNames: _ts.LanguageServiceHost['resolveModuleNames'] = (moduleNames: string[], containingFile: string, reusedNames: string[] | undefined, redirectedReference: _ts.ResolvedProjectReference | undefined, optionsOnlyWithNewerTsVersions: _ts.CompilerOptions): (_ts.ResolvedModule | undefined)[] => { | ||
return moduleNames.map(moduleName => { | ||
const { resolvedModule } = ts.resolveModuleName(moduleName, containingFile, config.options, serviceHost, moduleResolutionCache, redirectedReference) | ||
if (resolvedModule) { | ||
fixupResolvedModule(resolvedModule) | ||
} | ||
return resolvedModule | ||
}) | ||
} | ||
|
||
// language service never calls this, but TS docs recommend that we implement it | ||
const getResolvedModuleWithFailedLookupLocationsFromCache: _ts.LanguageServiceHost['getResolvedModuleWithFailedLookupLocationsFromCache'] = (moduleName, containingFile): _ts.ResolvedModuleWithFailedLookupLocations | undefined => { | ||
const ret = ts.resolveModuleNameFromCache(moduleName, containingFile, moduleResolutionCache) | ||
if (ret && ret.resolvedModule) { | ||
fixupResolvedModule(ret.resolvedModule) | ||
} | ||
return ret | ||
} | ||
|
||
const resolveTypeReferenceDirectives: _ts.LanguageServiceHost['resolveTypeReferenceDirectives'] = (typeDirectiveNames: string[], containingFile: string, redirectedReference: _ts.ResolvedProjectReference | undefined, options: _ts.CompilerOptions): (_ts.ResolvedTypeReferenceDirective | undefined)[] => { | ||
// Note: seems to be called with empty typeDirectiveNames array for all files. | ||
return typeDirectiveNames.map(typeDirectiveName => { | ||
const { resolvedTypeReferenceDirective } = ts.resolveTypeReferenceDirective(typeDirectiveName, containingFile, config.options, serviceHost, redirectedReference) | ||
if (resolvedTypeReferenceDirective) { | ||
fixupResolvedModule(resolvedTypeReferenceDirective) | ||
} | ||
return resolvedTypeReferenceDirective | ||
}) | ||
} | ||
|
||
return { | ||
resolveModuleNames, | ||
getResolvedModuleWithFailedLookupLocationsFromCache, | ||
resolveTypeReferenceDirectives, | ||
isFileKnownToBeInternal, | ||
markBucketOfFilenameInternal | ||
} | ||
} | ||
|
||
// Use full language services when the fast option is disabled. | ||
if (!transpileOnly) { | ||
const fileContents = new Map<string, string>() | ||
|
@@ -519,14 +627,15 @@ export function create (rawOptions: CreateOptions = {}): Register { | |
} | ||
|
||
// Create the compiler host for type checking. | ||
const serviceHost: _ts.LanguageServiceHost = { | ||
const serviceHost: _ts.LanguageServiceHost & Required<Pick<_ts.LanguageServiceHost, 'fileExists' | 'readFile'>> = { | ||
getProjectVersion: () => String(projectVersion), | ||
getScriptFileNames: () => Array.from(rootFileNames), | ||
getScriptVersion: (fileName: string) => { | ||
const version = fileVersions.get(fileName) | ||
return version ? version.toString() : '' | ||
}, | ||
getScriptSnapshot (fileName: string) { | ||
// TODO ordering of this with getScriptVersion? Should they sync up? | ||
let contents = fileContents.get(fileName) | ||
|
||
// Read contents into TypeScript memory cache. | ||
|
@@ -546,21 +655,27 @@ export function create (rawOptions: CreateOptions = {}): Register { | |
getDirectories: cachedLookup(debugFn('getDirectories', ts.sys.getDirectories)), | ||
fileExists: cachedLookup(debugFn('fileExists', fileExists)), | ||
directoryExists: cachedLookup(debugFn('directoryExists', ts.sys.directoryExists)), | ||
realpath: ts.sys.realpath ? cachedLookup(debugFn('realpath', ts.sys.realpath)) : undefined, | ||
getNewLine: () => ts.sys.newLine, | ||
useCaseSensitiveFileNames: () => ts.sys.useCaseSensitiveFileNames, | ||
getCurrentDirectory: () => cwd, | ||
getCompilationSettings: () => config.options, | ||
getDefaultLibFileName: () => ts.getDefaultLibFilePath(config.options), | ||
getCustomTransformers: getCustomTransformers | ||
} | ||
const { resolveModuleNames, getResolvedModuleWithFailedLookupLocationsFromCache, resolveTypeReferenceDirectives, isFileKnownToBeInternal, markBucketOfFilenameInternal } = createResolverFunctions(serviceHost) | ||
serviceHost.resolveModuleNames = resolveModuleNames | ||
serviceHost.getResolvedModuleWithFailedLookupLocationsFromCache = getResolvedModuleWithFailedLookupLocationsFromCache | ||
serviceHost.resolveTypeReferenceDirectives = resolveTypeReferenceDirectives | ||
|
||
const registry = ts.createDocumentRegistry(ts.sys.useCaseSensitiveFileNames, cwd) | ||
const service = ts.createLanguageService(serviceHost, registry) | ||
|
||
const updateMemoryCache = (contents: string, fileName: string) => { | ||
// Add to `rootFiles` if not already there | ||
// This is necessary to force TS to emit output | ||
if (!rootFileNames.has(fileName)) { | ||
// Add to `rootFiles` as necessary, either to make TS include a file it has not seen, | ||
// or to trigger a re-classification of files from external to internal. | ||
if (!rootFileNames.has(fileName) && !isFileKnownToBeInternal(fileName)) { | ||
markBucketOfFilenameInternal(fileName) | ||
rootFileNames.add(fileName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutating rootFiles seems to throw away a lot of internal compiler state. I don't know 100% how much, but it seems to cause a lot of resolutions to be re-performed, more-so than e.g. modifying an already-loaded source file to I think in general avoiding mutating rootFiles is going to be good for performance. |
||
// Increment project version for every change to rootFileNames. | ||
projectVersion++ | ||
|
@@ -632,13 +747,15 @@ export function create (rawOptions: CreateOptions = {}): Register { | |
return { name, comment } | ||
} | ||
} else { | ||
const sys = { | ||
const sys: _ts.System & _ts.FormatDiagnosticsHost = { | ||
...ts.sys, | ||
...diagnosticHost, | ||
readFile: (fileName: string) => { | ||
const cacheContents = fileContents.get(fileName) | ||
if (cacheContents !== undefined) return cacheContents | ||
return cachedReadFile(fileName) | ||
const contents = cachedReadFile(fileName) | ||
if (contents) fileContents.set(fileName, contents) | ||
return contents | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use |
||
}, | ||
readDirectory: ts.sys.readDirectory, | ||
getDirectories: cachedLookup(debugFn('getDirectories', ts.sys.getDirectories)), | ||
|
@@ -648,6 +765,7 @@ export function create (rawOptions: CreateOptions = {}): Register { | |
realpath: ts.sys.realpath ? cachedLookup(debugFn('realpath', ts.sys.realpath)) : undefined | ||
} | ||
|
||
const registry = ts.createDocumentRegistry(ts.sys.useCaseSensitiveFileNames, cwd) | ||
const host: _ts.CompilerHost = ts.createIncrementalCompilerHost | ||
? ts.createIncrementalCompilerHost(config.options, sys) | ||
: { | ||
|
@@ -661,6 +779,8 @@ export function create (rawOptions: CreateOptions = {}): Register { | |
getDefaultLibFileName: () => normalizeSlashes(join(dirname(compiler), ts.getDefaultLibFileName(config.options))), | ||
useCaseSensitiveFileNames: () => sys.useCaseSensitiveFileNames | ||
} | ||
const { resolveModuleNames, isFileKnownToBeInternal, markBucketOfFilenameInternal } = createResolverFunctions(host) | ||
host.resolveModuleNames = resolveModuleNames | ||
|
||
// Fallback for older TypeScript releases without incremental API. | ||
let builderProgram = ts.createIncrementalProgram | ||
|
@@ -687,17 +807,22 @@ export function create (rawOptions: CreateOptions = {}): Register { | |
|
||
// Set the file contents into cache manually. | ||
const updateMemoryCache = (contents: string, fileName: string) => { | ||
const sourceFile = builderProgram.getSourceFile(fileName) | ||
|
||
fileContents.set(fileName, contents) | ||
const previousContents = fileContents.get(fileName) | ||
const contentsChanged = previousContents !== contents | ||
if (contentsChanged) { | ||
fileContents.set(fileName, contents) | ||
} | ||
|
||
// Add to `rootFiles` when discovered by compiler for the first time. | ||
if (sourceFile === undefined) { | ||
let addedToRootFileNames = false | ||
if (!rootFileNames.has(fileName) && !isFileKnownToBeInternal(fileName)) { | ||
markBucketOfFilenameInternal(fileName) | ||
rootFileNames.add(fileName) | ||
addedToRootFileNames = true | ||
} | ||
|
||
// Update program when file changes. | ||
if (sourceFile === undefined || sourceFile.text !== contents) { | ||
if (addedToRootFileNames || contentsChanged) { | ||
builderProgram = ts.createEmitAndSemanticDiagnosticsBuilderProgram( | ||
Array.from(rootFileNames), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just being a bit smarter about when a file must be added to |
||
config.options, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.