Skip to content

Compare package.json paths with correct sensitivity in getLocalModuleSpecifier - #57973

Merged
Jake Bailey (jakebailey) merged 3 commits into
microsoft:mainfrom
jakebailey:weird-case-thing
Mar 28, 2024
Merged

Compare package.json paths with correct sensitivity in getLocalModuleSpecifier#57973
Jake Bailey (jakebailey) merged 3 commits into
microsoft:mainfrom
jakebailey:weird-case-thing

Conversation

@jakebailey

@jakebaileyJake Bailey (jakebailey) commented Mar 27, 2024

Copy link
Copy Markdown
Member

Fixes#57956
Fixes#57926
Fixes#57802
Closes#57961

Thanks Hang Liang (@xlianghang) for the test.

See the second commit for the change.

@jakebailey

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
pack this✅ Started✅ Results

@typescript-botTypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 27, 2024
Comment threadsrc/compiler/moduleSpecifiers.ts Outdated
Comment on lines +553 to +554
nearestTargetPackageJson &&= toPath(nearestTargetPackageJson, projectDirectory, getCanonicalFileName);
nearestSourcePackageJson &&= toPath(nearestSourcePackageJson, projectDirectory, getCanonicalFileName);

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear if getNearestAncestorDirectoryWithPackageJson should just always return Path.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if i should just comparePaths?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used elsewhere and a toPath might have other negative affects at those use sites.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean my Path comment, it's only used when reading a file so shouldn't actually affect anything, but it sounds like the comparison one may just be better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say just use comparePaths. We use it throughout tryGetModuleNameFromExportsOrImports against a path that comes from getNearestAncestorDirectoryWithPackageJson as well, so it would be consistent.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use a helper; slightly different than the one you suggested but I was having trouble understanding what I was reading...

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), 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/160790/artifacts?artifactName=tgz&fileId=2ECF6F93582C4D4C0D57F671D7B6546C54FCF3418117B22E972D91463EAC789C02&fileName=/typescript-5.5.0-insiders.20240327.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.5.0-pr-57973-2".;

@jakebaileyJake Bailey (jakebailey) changed the title Ensure package.json dir is compared after being toPath'dCompare package.json paths with correct sensitivityMar 27, 2024
@jakebaileyJake Bailey (jakebailey) changed the title Compare package.json paths with correct sensitivityCompare package.json paths with correct sensitivity in getLocalModuleSpecifierMar 27, 2024
@jakebailey

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
test top400✅ Started✅ Results
user test this✅ Started✅ Results
run dt✅ Started✅ Results
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,686k (± 0.01%)295,686k (± 0.01%)~295,658k295,714kp=0.936 n=6
Parse Time2.66s (± 0.31%)2.66s (± 0.28%)~2.65s2.67sp=0.306 n=6
Bind Time0.84s (± 0.97%)0.84s (± 1.06%)~0.83s0.85sp=0.550 n=6
Check Time8.26s (± 0.15%)8.22s (± 0.20%)-0.04s (- 0.42%)8.19s8.24sp=0.003 n=6
Emit Time7.04s (± 0.18%)7.05s (± 0.25%)~7.02s7.07sp=0.565 n=6
Total Time18.79s (± 0.09%)18.77s (± 0.05%)-0.02s (- 0.11%)18.75s18.78sp=0.046 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used194,461k (± 0.95%)193,352k (± 0.96%)~192,061k195,773kp=0.936 n=6
Parse Time1.65s (± 0.77%)1.66s (± 0.82%)~1.64s1.68sp=0.615 n=6
Bind Time0.87s (± 0.59%)0.88s (± 0.96%)~0.86s0.88sp=0.533 n=6
Check Time11.26s (± 0.41%)11.27s (± 0.35%)~11.22s11.31sp=0.629 n=6
Emit Time3.15s (± 0.71%)3.14s (± 0.79%)~3.11s3.17sp=0.571 n=6
Total Time16.93s (± 0.39%)16.94s (± 0.23%)~16.88s16.98sp=0.418 n=6
Monaco - node (v18.15.0, x64)
Memory used347,407k (± 0.01%)347,418k (± 0.01%)~347,385k347,480kp=0.689 n=6
Parse Time3.70s (± 0.97%)3.69s (± 1.31%)~3.61s3.76sp=1.000 n=6
Bind Time1.38s (± 1.86%)1.38s (± 0.54%)~1.37s1.39sp=0.548 n=6
Check Time10.24s (± 0.44%)10.23s (± 0.41%)~10.17s10.28sp=0.808 n=6
Emit Time6.04s (± 0.52%)6.00s (± 0.25%)-0.04s (- 0.61%)5.98s6.02sp=0.035 n=6
Total Time21.35s (± 0.36%)21.30s (± 0.10%)~21.28s21.33sp=0.173 n=6
TFS - node (v18.15.0, x64)
Memory used302,779k (± 0.01%)302,780k (± 0.01%)~302,749k302,810kp=1.000 n=6
Parse Time2.40s (± 1.34%)2.42s (± 0.76%)~2.39s2.44sp=0.169 n=6
Bind Time1.19s (± 0.86%)1.22s (± 0.52%)+0.03s (+ 2.23%)1.21s1.23sp=0.005 n=6
Check Time7.51s (± 0.11%)7.50s (± 0.33%)~7.46s7.53sp=0.163 n=6
Emit Time4.26s (± 0.44%)4.27s (± 0.75%)~4.23s4.32sp=0.686 n=6
Total Time15.36s (± 0.13%)15.40s (± 0.39%)~15.34s15.50sp=0.295 n=6
material-ui - node (v18.15.0, x64)
Memory used510,144k (± 0.01%)510,085k (± 0.01%)~510,038k510,201kp=0.093 n=6
Parse Time3.18s (± 0.47%)3.20s (± 0.55%)~3.17s3.22sp=0.217 n=6
Bind Time1.18s (± 1.07%)1.18s (± 0.44%)~1.18s1.19sp=0.931 n=6
Check Time20.56s (± 0.25%)20.55s (± 0.21%)~20.50s20.62sp=0.630 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time24.92s (± 0.26%)24.93s (± 0.23%)~24.87s25.01sp=0.936 n=6
mui-docs - node (v18.15.0, x64)
Memory used1,744,452k (± 0.00%)1,744,451k (± 0.00%)~1,744,399k1,744,519kp=0.936 n=6
Parse Time9.59s (± 0.60%)9.63s (± 0.64%)~9.53s9.69sp=0.376 n=6
Bind Time3.45s (± 0.91%)3.46s (± 0.93%)~3.41s3.51sp=0.626 n=6
Check Time81.72s (± 0.34%)81.66s (± 0.52%)~81.05s82.28sp=0.936 n=6
Emit Time0.19s (± 2.13%)0.19s (± 0.00%)~0.19s0.19sp=0.405 n=6
Total Time94.96s (± 0.27%)94.94s (± 0.46%)~94.30s95.64sp=0.936 n=6
self-build-src - node (v18.15.0, x64)
Memory used2,396,078k (± 0.03%)2,395,862k (± 0.02%)~2,395,030k2,396,842kp=0.689 n=6
Parse Time6.00s (± 0.91%)6.00s (± 1.08%)~5.93s6.09sp=1.000 n=6
Bind Time2.26s (± 1.23%)2.29s (± 1.12%)~2.27s2.34sp=0.127 n=6
Check Time39.89s (± 0.22%)39.90s (± 0.31%)~39.76s40.06sp=0.936 n=6
Emit Time3.14s (± 1.68%)3.15s (± 1.19%)~3.11s3.22sp=0.936 n=6
Total Time51.31s (± 0.28%)51.38s (± 0.20%)~51.23s51.48sp=0.810 n=6
self-compiler - node (v18.15.0, x64)
Memory used415,839k (± 0.01%)415,882k (± 0.02%)~415,803k416,049kp=0.378 n=6
Parse Time4.10s (± 0.33%)4.12s (± 0.94%)~4.06s4.17sp=0.371 n=6
Bind Time1.59s (± 0.62%)1.58s (± 1.11%)~1.55s1.60sp=0.117 n=6
Check Time22.54s (± 0.16%)22.53s (± 0.35%)~22.43s22.61sp=0.872 n=6
Emit Time1.72s (± 1.80%)1.69s (± 1.59%)~1.67s1.74sp=0.107 n=6
Total Time29.96s (± 0.11%)29.92s (± 0.37%)~29.80s30.05sp=0.689 n=6
vscode - node (v18.15.0, x64)
Memory used2,896,201k (± 0.00%)2,896,256k (± 0.00%)+56k (+ 0.00%)2,896,149k2,896,322kp=0.045 n=6
Parse Time12.94s (± 0.32%)13.42s (± 8.52%)~12.89s15.76sp=0.255 n=6
Bind Time4.14s (± 0.33%)4.15s (± 0.10%)~4.14s4.15sp=0.078 n=6
Check Time72.03s (± 0.29%)71.93s (± 0.34%)~71.65s72.20sp=0.689 n=6
Emit Time19.37s (± 0.47%)19.42s (± 0.76%)~19.29s19.62sp=1.000 n=6
Total Time108.48s (± 0.16%)108.92s (± 1.18%)~108.02s111.48sp=0.873 n=6
webpack - node (v18.15.0, x64)
Memory used408,874k (± 0.02%)408,918k (± 0.04%)~408,774k409,181kp=0.936 n=6
Parse Time3.89s (± 0.68%)3.88s (± 0.55%)~3.86s3.92sp=0.468 n=6
Bind Time1.69s (± 0.81%)1.69s (± 1.06%)~1.67s1.72sp=0.935 n=6
Check Time16.85s (± 0.18%)16.88s (± 0.38%)~16.80s16.96sp=0.575 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time22.43s (± 0.10%)22.45s (± 0.35%)~22.35s22.55sp=0.627 n=6
xstate - node (v18.15.0, x64)
Memory used513,400k (± 0.02%)513,364k (± 0.01%)~513,276k513,458kp=0.936 n=6
Parse Time3.28s (± 0.23%)3.28s (± 0.33%)~3.27s3.30sp=0.604 n=6
Bind Time1.57s (± 0.00%)1.57s (± 0.26%)~1.57s1.58sp=0.405 n=6
Check Time2.90s (± 0.56%)2.90s (± 0.49%)~2.88s2.92sp=0.745 n=6
Emit Time0.07s (± 7.03%)0.07s (± 5.69%)~0.07s0.08sp=0.595 n=6
Total Time7.82s (± 0.15%)7.82s (± 0.21%)~7.80s7.85sp=0.806 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@jakebailey

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) cherry-pick this to release-5.4

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
cherry-pick this to release-5.4✅ Started✅ Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey, Jake Bailey (@jakebailey)! I've created #57976 for you.

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user tests comparing main and refs/pull/57973/merge:

Everything looks good!

function packageJsonPathsAreEqual(a: string | undefined, b: string | undefined, ignoreCase?: boolean) {
if (a === b) return true;
if (a === undefined || b === undefined) return false;
return comparePaths(a, b, ignoreCase) === Comparison.EqualTo;

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.

dont you need to pass current directory for this to work correctly

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These paths are already absolute, it seems, so nothing broke...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the other use in this file and they don't pass in the working dir either (also for paths from this host method)

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the top 400 repos comparing main and refs/pull/57973/merge:

Everything looks good!

@andrewbranch

Copy link
Copy Markdown
Member

Fixes #57802

@typescript-botTypeScript Bot (typescript-bot) added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 28, 2024
@jakebailey
Jake Bailey (jakebailey) merged commit 35f4f03 into microsoft:mainMar 28, 2024
@jakebailey
Jake Bailey (jakebailey) deleted the weird-case-thing branch March 28, 2024 17:33
Daniel Rosenwasser (DanielRosenwasser) pushed a commit that referenced this pull request Mar 28, 2024
…e-5.4 (#57976)
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
@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

7 participants

@jakebailey@typescript-bot@andrewbranch@rbuckton@RyanCavanaugh@sheetalkamat@DanielRosenwasser