-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(check/lsp): fix bugs with tsc type resolution, allow npm packages to augment ImportMeta
#27690
Changes from all commits
132725d
5a75c7e
b218c6d
5eec3da
6f8f41d
707a254
b6598b3
ee96d48
f391221
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 |
---|---|---|
|
@@ -500,6 +500,8 @@ delete Object.prototype.__proto__; | |
// Microsoft/TypeScript#26825 but that doesn't seem to be working here, | ||
// so we will ignore complaints about this compiler setting. | ||
5070, | ||
// TS6053: File '{0}' not found. | ||
6053, | ||
// TS7016: Could not find a declaration file for module '...'. '...' | ||
// implicitly has an 'any' type. This is due to `allowJs` being off by | ||
// default but importing of a JavaScript module. | ||
|
@@ -705,15 +707,14 @@ delete Object.prototype.__proto__; | |
resolveTypeReferenceDirectiveReferences( | ||
typeDirectiveReferences, | ||
containingFilePath, | ||
redirectedReference, | ||
_redirectedReference, | ||
options, | ||
containingSourceFile, | ||
_reusedNames, | ||
) { | ||
const isCjs = | ||
containingSourceFile?.impliedNodeFormat === ts.ModuleKind.CommonJS; | ||
/** @type {Array<ts.ResolvedTypeReferenceDirectiveWithFailedLookupLocations>} */ | ||
const result = typeDirectiveReferences.map((arg) => { | ||
const toResolve = typeDirectiveReferences.map((arg) => { | ||
/** @type {ts.FileReference} */ | ||
const fileReference = typeof arg === "string" | ||
? { | ||
|
@@ -722,46 +723,50 @@ delete Object.prototype.__proto__; | |
fileName: arg, | ||
} | ||
: arg; | ||
if (fileReference.fileName.startsWith("npm:")) { | ||
/** @type {[string, ts.Extension | null] | undefined} */ | ||
const resolved = ops.op_resolve( | ||
containingFilePath, | ||
[ | ||
[ | ||
fileReference.resolutionMode == null | ||
? isCjs | ||
: fileReference.resolutionMode === ts.ModuleKind.CommonJS, | ||
fileReference.fileName, | ||
], | ||
], | ||
)?.[0]; | ||
if (resolved && resolved[1]) { | ||
return { | ||
resolvedTypeReferenceDirective: { | ||
primary: true, | ||
resolvedFileName: resolved[0], | ||
// todo(dsherret): we should probably be setting this | ||
isExternalLibraryImport: undefined, | ||
}, | ||
}; | ||
} else { | ||
return { | ||
resolvedTypeReferenceDirective: undefined, | ||
}; | ||
} | ||
return [ | ||
fileReference.resolutionMode == null | ||
? isCjs | ||
: fileReference.resolutionMode === ts.ModuleKind.CommonJS, | ||
fileReference.fileName, | ||
]; | ||
}); | ||
|
||
/** @type {Array<[string, ts.Extension | null] | undefined>} */ | ||
const resolved = ops.op_resolve( | ||
containingFilePath, | ||
toResolve, | ||
); | ||
|
||
/** @type {Array<ts.ResolvedTypeReferenceDirectiveWithFailedLookupLocations>} */ | ||
const result = resolved.map((item) => { | ||
if (item && item[1]) { | ||
const [resolvedFileName, extension] = item; | ||
return { | ||
resolvedTypeReferenceDirective: { | ||
primary: true, | ||
resolvedFileName, | ||
extension, | ||
isExternalLibraryImport: false, | ||
}, | ||
}; | ||
} else { | ||
return ts.resolveTypeReferenceDirective( | ||
fileReference.fileName, | ||
containingFilePath, | ||
options, | ||
host, | ||
redirectedReference, | ||
undefined, | ||
containingSourceFile?.impliedNodeFormat ?? | ||
fileReference.resolutionMode, | ||
); | ||
return { | ||
resolvedTypeReferenceDirective: undefined, | ||
}; | ||
} | ||
}); | ||
|
||
if (logDebug) { | ||
debug( | ||
"resolveTypeReferenceDirectiveReferences ", | ||
typeDirectiveReferences, | ||
containingFilePath, | ||
options, | ||
containingSourceFile?.fileName, | ||
" => ", | ||
result, | ||
); | ||
} | ||
return result; | ||
}, | ||
resolveModuleNameLiterals( | ||
|
@@ -1116,6 +1121,36 @@ delete Object.prototype.__proto__; | |
if (IGNORED_DIAGNOSTICS.includes(diagnostic.code)) { | ||
return false; | ||
} | ||
|
||
// ignore diagnostics resulting from the `ImportMeta` declaration in deno merging with | ||
// the one in @types/node. the types of the filename and dirname properties are different, | ||
// which causes tsc to error. | ||
const importMetaFilenameDirnameModifiersRe = | ||
/^All declarations of '(filename|dirname)'/; | ||
const importMetaFilenameDirnameTypesRe = | ||
/^Subsequent property declarations must have the same type.\s+Property '(filename|dirname)'/; | ||
// Declarations of X must have identical modifiers. | ||
if (diagnostic.code === 2687) { | ||
if ( | ||
typeof diagnostic.messageText === "string" && | ||
(importMetaFilenameDirnameModifiersRe.test(diagnostic.messageText)) && | ||
(diagnostic.file?.fileName.startsWith("asset:///") || | ||
diagnostic.file?.fileName?.includes("@types/node")) | ||
) { | ||
return false; | ||
} | ||
} | ||
// Subsequent property declarations must have the same type. | ||
if (diagnostic.code === 2717) { | ||
if ( | ||
typeof diagnostic.messageText === "string" && | ||
(importMetaFilenameDirnameTypesRe.test(diagnostic.messageText)) && | ||
(diagnostic.file?.fileName.startsWith("asset:///") || | ||
diagnostic.file?.fileName?.includes("@types/node")) | ||
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 work for the global cache? 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. It seems to 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. Added a test for this (across all of the node modules modes) 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, I misremembered how the global cache was structured. |
||
) { | ||
return false; | ||
} | ||
} | ||
// make the diagnostic for using an `export =` in an es module a warning | ||
if (diagnostic.code === 1203) { | ||
diagnostic.category = ts.DiagnosticCategory.Warning; | ||
|
@@ -1410,7 +1445,6 @@ delete Object.prototype.__proto__; | |
"ErrorConstructor", | ||
"gc", | ||
"Global", | ||
"ImportMeta", | ||
"localStorage", | ||
"queueMicrotask", | ||
"RequestInit", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
/// <reference types="./real-import-meta.d.ts" /> | ||
|
||
export type Foo = number; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,13 @@ | ||
{ | ||
"name": "@denotest/augments-global", | ||
"version": "1.0.0", | ||
"types": "./index.d.ts" | ||
"types": "./index.d.ts", | ||
"exports": { | ||
".": { | ||
"types": "./index.d.ts" | ||
}, | ||
"./import-meta": { | ||
"types": "./import-meta.d.ts" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
interface ImportMetaEnv { | ||
TEST: string; | ||
} | ||
|
||
interface ImportMeta { | ||
env: ImportMetaEnv; | ||
bar: number; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
{ | ||
"tempDir": true, | ||
"tests": { | ||
"node_modules_dir_none": { | ||
"steps": [ | ||
{ | ||
"args": "run -A ./set_node_modules_dir.ts none", | ||
"output": "" | ||
}, | ||
{ | ||
"args": "install", | ||
"output": "[WILDCARD]" | ||
}, | ||
{ | ||
"args": "check --all ./main.ts", | ||
"output": "Check [WILDCARD]main.ts\n" | ||
} | ||
] | ||
}, | ||
"node_modules_dir_auto": { | ||
"steps": [ | ||
{ | ||
"args": "run -A ./set_node_modules_dir.ts auto", | ||
"output": "" | ||
}, | ||
{ | ||
"args": "install", | ||
"output": "[WILDCARD]" | ||
}, | ||
{ | ||
"args": "check --all ./main.ts", | ||
"output": "Check [WILDCARD]main.ts\n" | ||
} | ||
] | ||
}, | ||
"node_modules_dir_manual": { | ||
"steps": [ | ||
{ | ||
"args": "run -A ./set_node_modules_dir.ts auto", | ||
"output": "" | ||
}, | ||
{ | ||
"args": "install", | ||
"output": "[WILDCARD]" | ||
}, | ||
{ | ||
"args": "check --all ./main.ts", | ||
"output": "Check [WILDCARD]main.ts\n" | ||
} | ||
] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"imports": { | ||
"@types/node": "npm:@types/node@*" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
/// <reference types="@types/node" /> | ||
|
||
const _foo = import.meta.dirname; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
if (Deno.args.length !== 1) { | ||
console.error("Usage: set_node_modules_dir.ts <setting>"); | ||
Deno.exit(1); | ||
} | ||
const setting = Deno.args[0].trim(); | ||
const denoJson = JSON.parse(Deno.readTextFileSync("./deno.json")); | ||
denoJson["nodeModulesDir"] = setting; | ||
Deno.writeTextFileSync("./deno.json", JSON.stringify(denoJson, null, 2)); |
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.
Huh, I kept trying this as a hail mary for some other bugs but could never manifest a case where tsc was constructing paths bypassing
op_resolve()
(which typically stores mappings for every specifier when denormalizing its results). I guess it's happening athost.resolveTypeReferenceDirectiveReferences()
.Nice find!