-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Validate JSON imports into ESM in --module nodenext
#60019
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
Conversation
// In Node.js, JSON modules don't get named exports | ||
if (ModuleKind.Node16 <= moduleKind && moduleKind <= ModuleKind.NodeNext) { | ||
const usageMode = getEmitSyntaxForModuleSpecifierExpression(usage); | ||
return usageMode === ModuleKind.ESNext && endsWith((usage as StringLiteralLike).text, Extension.Json); |
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.
The text of the module specifier is not sufficient to know whether we're importing a JSON file; this produced both false positives and false negatives (see imports of "actually-json"
and "not.json"
in the test)
if (index >= 0) { | ||
return fileName.substring(index); | ||
return baseName.substring(index); |
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 was extremely bad; I guess we were only really using this to determine whether something was a declaration file name, not to actually use the extension.
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Fixes #59684
This does two main things:
--module nodenext
, since Node.js crashes without them--module nodenext
, since those don't work in Node.js. We already sort of considered this information when building up the type of a namespace import of a JSON file, but named imports bypassed that check.