Skip to content

Only look up package.json type if module is node16/nodenext or file is in node_modules - #58825

Merged
Andrew Branch (andrewbranch) merged 7 commits into
microsoft:mainfrom
andrewbranch:package-json-type-only-in-node_modules
Jul 17, 2024
Merged

Only look up package.json type if module is node16/nodenext or file is in node_modules#58825
Andrew Branch (andrewbranch) merged 7 commits into
microsoft:mainfrom
andrewbranch:package-json-type-only-in-node_modules

Conversation

@andrewbranch

@andrewbranchAndrew Branch (andrewbranch) commented Jun 10, 2024

Copy link
Copy Markdown
Member

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 of node_modules dependencies, but with some effects on local files, e.g. with verbatimModuleSyntax) 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 is node16/nodenext (as we’ve always done) or if the file is inside node_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 like

.
└── packages/
├── app/
│ ├── node_modules/shared ↪︎
│ ├── tsconfig.json
│ ├── package.json
│ └── index.ts
└── shared/
├── dist/index.d.ts
├── src/index.ts
├── tsconfig.json
└── package.json

When we build shared/tsconfig.json, we decide that shared/src/index.ts is not inside node_modules. Then, when we build app/tsconfig.json, even though we resolve app/node_modules/shared/dist/index.d.ts, we realpath it to shared/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 of app/tsconfig.json during development to behave differently than it would after publishing/deploying into a situation where app/node_modules/shared is no longer a symlink, but a real node_modules package.

Fixes#54752
Fixes#50647

@andrewbranch

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) test top400

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 11, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
test top400✅ Started✅ Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Andrew Branch (@andrewbranch) Here are the results of running the top 400 repos with tsc comparing main and refs/pull/58825/merge:

Everything looks good!

@andrewbranch

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 11, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
pack this✅ Started✅ Results

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 11, 2024

Copy link
Copy Markdown
Contributor

Hey Andrew Branch (@andrewbranch), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/162168/artifacts?artifactName=tgz&fileId=C1ACA7462D57C70E93C4D3280AA7732A4C19585806197BFA46B31559D77CD4B402&fileName=/typescript-5.6.0-insiders.20240611.tgz"
}
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.6.0-pr-58825-5".;

@andrewbranch

Copy link
Copy Markdown
MemberAuthor

Perf should be unchanged or better, but

TypeScript Bot (@typescript-bot) perf test faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 11, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
perf test faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Andrew Branch (@andrewbranch)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Compiler-Unions - node (v18.15.0, x64)
Errors3030~~~p=1.000 n=6
Symbols62,15362,153~~~p=1.000 n=6
Types50,24250,242~~~p=1.000 n=6
Memory used193,725k (± 0.89%)193,513k (± 0.90%)~192,250k195,779kp=0.230 n=6
Parse Time1.30s (± 0.79%)1.30s (± 0.64%)~1.29s1.31sp=0.923 n=6
Bind Time0.72s0.72s~~~p=1.000 n=6
Check Time9.58s (± 0.35%)9.58s (± 0.61%)~9.53s9.68sp=0.936 n=6
Emit Time2.77s (± 0.37%)2.76s (± 0.70%)~2.74s2.79sp=0.325 n=6
Total Time14.37s (± 0.28%)14.37s (± 0.44%)~14.31s14.45sp=0.871 n=6
angular-1 - node (v18.15.0, x64)
Errors55~~~p=1.000 n=6
Symbols944,109944,109~~~p=1.000 n=6
Types407,049407,049~~~p=1.000 n=6
Memory used1,222,155k (± 0.00%)1,219,694k (± 0.00%)-2,462k (- 0.20%)1,219,639k1,219,782kp=0.005 n=6
Parse Time8.09s (± 0.47%)8.04s (± 0.18%)~8.02s8.06sp=0.064 n=6
Bind Time2.24s (± 0.69%)2.26s (± 0.40%)+0.02s (+ 0.89%)2.25s2.27sp=0.039 n=6
Check Time36.63s (± 0.37%)36.44s (± 0.43%)~36.25s36.67sp=0.054 n=6
Emit Time18.07s (± 0.40%)17.90s (± 0.55%)-0.17s (- 0.96%)17.78s18.03sp=0.013 n=6
Total Time65.03s (± 0.26%)64.64s (± 0.20%)-0.39s (- 0.60%)64.46s64.76sp=0.005 n=6
mui-docs - node (v18.15.0, x64)
Errors55~~~p=1.000 n=6
Symbols2,131,8762,131,876~~~p=1.000 n=6
Types925,869925,869~~~p=1.000 n=6
Memory used2,120,037k (± 0.00%)2,116,210k (± 0.01%)-3,827k (- 0.18%)2,116,098k2,116,356kp=0.005 n=6
Parse Time6.83s (± 0.17%)6.69s (± 0.30%)-0.14s (- 2.05%)6.66s6.72sp=0.005 n=6
Bind Time2.29s (± 0.18%)2.32s (± 0.79%)+0.03s (+ 1.31%)2.29s2.34sp=0.016 n=6
Check Time71.30s (± 0.38%)71.26s (± 0.44%)~70.70s71.64sp=0.936 n=6
Emit Time0.14s0.14s~~~p=1.000 n=6
Total Time80.56s (± 0.34%)80.41s (± 0.37%)~79.87s80.74sp=0.378 n=6
self-build-src - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols1,230,4241,230,425+1 (+ 0.00%)~~p=0.001 n=6
Types261,178261,178~~~p=1.000 n=6
Memory used2,376,991k (± 2.59%)2,351,721k (± 0.06%)~2,350,180k2,353,055kp=0.689 n=6
Parse Time4.98s (± 0.93%)4.97s (± 0.89%)~4.91s5.04sp=0.810 n=6
Bind Time1.90s (± 0.61%)1.89s (± 0.33%)~1.88s1.90sp=0.070 n=6
Check Time34.11s (± 0.39%)34.12s (± 0.41%)~33.98s34.35sp=0.873 n=6
Emit Time2.64s (± 0.79%)2.66s (± 2.65%)~2.57s2.74sp=0.378 n=6
Total Time43.65s (± 0.34%)43.66s (± 0.26%)~43.48s43.82sp=0.575 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols1,230,4241,230,425+1 (+ 0.00%)~~p=0.001 n=6
Types261,178261,178~~~p=1.000 n=6
Memory used2,427,658k (± 0.02%)2,427,123k (± 0.05%)~2,425,891k2,428,927kp=0.298 n=6
Parse Time5.17s (± 0.39%)5.19s (± 1.04%)~5.14s5.26sp=1.000 n=6
Bind Time1.68s (± 0.31%)1.69s (± 0.75%)~1.68s1.71sp=0.418 n=6
Check Time34.51s (± 0.29%)34.57s (± 0.22%)~34.49s34.70sp=0.423 n=6
Emit Time2.65s (± 2.76%)2.70s (± 2.62%)~2.58s2.77sp=0.173 n=6
Total Time44.02s (± 0.35%)44.17s (± 0.19%)~44.08s44.31sp=0.065 n=6
self-compiler - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols259,181259,182+1 (+ 0.00%)~~p=0.001 n=6
Types105,173105,173~~~p=1.000 n=6
Memory used428,866k (± 0.03%)428,867k (± 0.01%)~428,827k428,913kp=0.378 n=6
Parse Time4.07s (± 0.65%)4.09s (± 1.00%)~4.03s4.15sp=0.517 n=6
Bind Time1.63s (± 0.74%)1.62s (± 1.49%)~1.59s1.65sp=0.622 n=6
Check Time22.27s (± 0.67%)22.38s (± 0.30%)~22.27s22.46sp=0.228 n=6
Emit Time1.74s (± 1.01%)1.72s (± 1.41%)~1.69s1.76sp=0.126 n=6
Total Time29.72s (± 0.47%)29.80s (± 0.39%)~29.60s29.89sp=0.376 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors3535~~~p=1.000 n=6
Symbols224,565224,565~~~p=1.000 n=6
Types93,73493,734~~~p=1.000 n=6
Memory used369,796k (± 0.04%)369,638k (± 0.03%)-158k (- 0.04%)369,515k369,775kp=0.045 n=6
Parse Time3.46s (± 1.35%)3.46s (± 0.41%)~3.44s3.48sp=0.936 n=6
Bind Time1.95s (± 0.96%)1.95s (± 2.12%)~1.89s2.01sp=0.807 n=6
Check Time19.36s (± 0.28%)19.40s (± 0.19%)~19.35s19.44sp=0.148 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time24.77s (± 0.31%)24.82s (± 0.16%)~24.74s24.85sp=0.224 n=6
vscode - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols2,852,9762,852,976~~~p=1.000 n=6
Types967,867967,867~~~p=1.000 n=6
Memory used3,025,016k (± 0.00%)3,020,070k (± 0.00%)-4,946k (- 0.16%)3,019,976k3,020,204kp=0.005 n=6
Parse Time13.82s (± 0.16%)13.65s (± 0.40%)-0.18s (- 1.30%)13.57s13.73sp=0.005 n=6
Bind Time4.27s (± 2.62%)4.17s (± 0.25%)~4.16s4.19sp=0.406 n=6
Check Time74.97s (± 2.49%)73.60s (± 0.32%)~73.27s73.96sp=0.066 n=6
Emit Time22.53s (± 8.36%)23.90s (± 0.77%)~23.69s24.12sp=0.054 n=6
Total Time115.59s (± 0.17%)115.32s (± 0.31%)~114.88s115.77sp=0.173 n=6
webpack - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols266,914266,914~~~p=1.000 n=6
Types108,685108,685~~~p=1.000 n=6
Memory used411,894k (± 0.02%)411,499k (± 0.02%)-396k (- 0.10%)411,379k411,655kp=0.005 n=6
Parse Time3.82s (± 1.09%)3.83s (± 0.59%)~3.80s3.86sp=0.627 n=6
Bind Time1.66s (± 0.99%)1.69s (± 0.65%)+0.03s (+ 2.11%)1.67s1.70sp=0.007 n=6
Check Time17.01s (± 0.45%)16.98s (± 0.42%)~16.84s17.03sp=0.688 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time22.49s (± 0.35%)22.51s (± 0.38%)~22.34s22.58sp=0.809 n=6
xstate-main - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols523,429523,429~~~p=1.000 n=6
Types177,955177,955~~~p=1.000 n=6
Memory used461,890k (± 0.02%)461,564k (± 0.07%)~461,216k461,953kp=0.093 n=6
Parse Time3.15s (± 0.68%)3.15s (± 0.55%)~3.12s3.17sp=0.872 n=6
Bind Time1.18s (± 0.46%)1.19s (± 0.69%)~1.18s1.20sp=0.859 n=6
Check Time18.21s (± 0.20%)18.14s (± 0.83%)~17.95s18.32sp=0.568 n=6
Emit Time0.00s (±244.70%)0.00s~~~p=0.405 n=6
Total Time22.55s (± 0.24%)22.47s (± 0.66%)~22.30s22.65sp=0.520 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@andrewbranch
Andrew Branch (andrewbranch)force-pushed the package-json-type-only-in-node_modules branch from 2677b01 to 6208d2fCompareJune 17, 2024 18:55
@typescript-bot

Copy link
Copy Markdown
Contributor

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 Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up.

Copy link
Copy Markdown
Member

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.

@andrewbranch

Copy link
Copy Markdown
MemberAuthor

We don't need these new ModuleSpecifierResolutionHost members to be public

Wesley Wigham (@weswigham)ModuleSpecifierResolutionHost itself is internal.

Comment threadsrc/compiler/checker.ts
Comment threadsrc/compiler/utilities.ts Outdated
Comment threadsrc/compiler/utilities.ts Outdated
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
@andrewbranch

Copy link
Copy Markdown
MemberAuthor

There’s a knip failure but it’s public API?

@weswigham

Copy link
Copy Markdown
Member

ModuleSpecifierResolutionHost itself is internal.

The specifics are internal, yes, but we expose and consume public objects and interfaces that we use as and expect to be ModuleSpecifierResolutionHosts which cannot be correctly implemented without these being made public in some way. There's a definite reason almost everything on ModuleSpecifierResolutionHost is optional. For example, Program has a second (@internal) declaration declaring it extends ModuleSpecifierResolutionHost. If there are any 3rd-party Programs (like our LS or builder construct internally to orchestrate builds, but external), they won't know they need these new methods on their Program to satisfy ModuleSpecifierResolutionHost uses, and since they're required, we'll just crash at runtime when they're inevitably missing.

@andrewbranch

Andrew Branch (andrewbranch) commented Jul 1, 2024

Copy link
Copy Markdown
MemberAuthor

There are 43 non-optional internal methods and properties declared on the public Program interface declaration—we haven’t made it possible to implement a third-party Program up from its public interface.

@andrewbranch

Copy link
Copy Markdown
MemberAuthor

Wesley Wigham (@weswigham) any other feedback on the API (or any of the PR)? I’d like to get this in soon.

@andrewbranch
Andrew Branch (andrewbranch) merged commit a9139bf into microsoft:mainJul 17, 2024
@andrewbranch
Andrew Branch (andrewbranch) deleted the package-json-type-only-in-node_modules branch July 17, 2024 16:23
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ESM-only packages come with synthesized default in moduleResolution: bundler Typescript [4.8.2] is adding invalid javascript for *.cjs files

6 participants

@andrewbranch@typescript-bot@weswigham@jakebailey@sheetalkamat@sandersn