-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Only look up package.json type if module is node16/nodenext or file is in node_modules #58825
Only look up package.json type if module is node16/nodenext or file is in node_modules #58825
Conversation
@typescript-bot test top400 |
@andrewbranch Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
@typescript-bot pack this |
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
Perf should be unchanged or better, but @typescript-bot perf test faster |
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
…fic file extensions in more `module` modes (microsoft#57896)"" This reverts commit d3c8f08.
…ExportInfoMap (microsoft#58837)"" This reverts commit a19bf0c.
2677b01
to
6208d2f
Compare
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
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.
We don't need these new ModuleSpecifierResolutionHost
members to be public because we automatically attach them or have fallbacks, right? Because these'll be missing from a user-provided host
object. We probably wannt mark 'em as optional and handle appropriately if they're non-public.
@weswigham |
Co-authored-by: Jake Bailey <[email protected]>
There’s a knip failure but it’s public API? |
The specifics are internal, yes, but we expose and consume public objects and interfaces that we use as and expect to be |
There are 43 non-optional internal methods and properties declared on the public |
@weswigham any other feedback on the API (or any of the PR)? I’d like to get this in soon. |
This walks back the heuristics added to non-Node.js module modes in #57896 a bit, based mostly on feedback in #58663. In 5.5-beta, an explicit package.json
"type"
or module-format-specific file extension started overriding"module": "commonjs"
or"module": "esnext"
in tsconfig.json. This impacted both type checking (relevant primarily for imports ofnode_modules
dependencies, but with some effects on local files, e.g. withverbatimModuleSyntax
) and emit (relevant for local project source files, if emit was enabled). The feedback in #58663 was that the effects on local files were unwelcome because the emitted files (whether from tsc or another tool) were going to be placed in some other context tsc didn’t know about, so the package.json tsc was finding during compilation shouldn’t be considered during the build.With this PR, we only look up package.json
"type"
if--module
isnode16
/nodenext
(as we’ve always done) or if the file is insidenode_modules
.Some testing and experimentation is still needed to ensure this doesn’t create inconsistencies with symlinked monorepos.This doesn’t seem to cause inconsistencies with workspaces-style monorepos, because module resolution takes the realpath of files in node_modules before we see if we need to look up a package.json"type"
. So in a project references example likeWhen we build
shared/tsconfig.json
, we decide thatshared/src/index.ts
is not inside node_modules. Then, when we buildapp/tsconfig.json
, even though we resolveapp/node_modules/shared/dist/index.d.ts
, we realpath it toshared/dist/index.d.ts
and count it as not inside node_modules. Having this consistency between input and output files regardless of how they’re resolved is what matters most. The caveat is that the realpath and project references logic can cause the type checking ofapp/tsconfig.json
during development to behave differently than it would after publishing/deploying into a situation whereapp/node_modules/shared
is no longer a symlink, but a real node_modules package.Fixes #54752
Fixes #50647