Skip to content
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

Do not lowercase typeReference directive name #58525

Merged
merged 4 commits into from
May 13, 2024
Merged

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 13, 2024

As part of #9824 all type reference directives are lower cased so type="somePackage" results in looking for type reference of somepackage instead.

We also did not lowercase file names found as part of auto type references. So there was always some descripency with how we handled typeRef. Now with this change, it's as written.

I reverted that part and only kept the jsTypings to do the lowercasing to find the @types packages to install.
This fixes issues with forceConsistentCasingInFileNames esp when using relative paths in types reference.

#26948 becomes absolute with this
Fixes #45096

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 13, 2024
@sheetalkamat
Copy link
Member Author

@typescript-bot test this
@typescript-bot test top100
@typescript-bot test tsserver top100
@typescript-bot user test this
@typescript-bot user test tsserver
@typescript-bot run dt
@typescript-bot perf test this
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 13, 2024

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

Command Status Results
test top100 ✅ Started ✅ Results
test tsserver top100 ✅ Started
user test this ✅ Started ✅ Results
user test tsserver ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this ✅ Started 👀 Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 13, 2024

Hey @sheetalkamat, 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/161724/artifacts?artifactName=tgz&fileId=B651C045846EEB2FC90BE3CE603705831C8A890856CD515773A0DF0BF360115702&fileName=/typescript-5.5.0-insiders.20240513.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-58525-2".;

@typescript-bot
Copy link
Collaborator

Hey @sheetalkamat, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@sheetalkamat Here are the results of running the user tests comparing main and refs/pull/58525/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

@sheetalkamat Here are the results of running the user tests comparing main and refs/pull/58525/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@sheetalkamat
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 30 30 ~ ~ ~ p=1.000 n=6
Symbols 62,154 62,154 ~ ~ ~ p=1.000 n=6
Types 50,248 50,248 ~ ~ ~ p=1.000 n=6
Memory used 192,857k (± 0.77%) 194,049k (± 1.02%) ~ 192,199k 195,916k p=0.689 n=6
Parse Time 1.55s (± 0.63%) 1.54s (± 1.77%) ~ 1.51s 1.58s p=0.935 n=6
Bind Time 0.87s (± 1.19%) 0.87s (± 0.97%) ~ 0.86s 0.88s p=0.923 n=6
Check Time 11.30s (± 0.52%) 11.30s (± 0.32%) ~ 11.27s 11.37s p=0.688 n=6
Emit Time 3.14s (± 0.81%) 3.14s (± 0.84%) ~ 3.10s 3.18s p=1.000 n=6
Total Time 16.86s (± 0.52%) 16.85s (± 0.19%) ~ 16.82s 16.90s p=0.466 n=6
angular-1 - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 944,110 944,110 ~ ~ ~ p=1.000 n=6
Types 407,140 407,140 ~ ~ ~ p=1.000 n=6
Memory used 1,222,071k (± 0.00%) 1,222,049k (± 0.00%) ~ 1,221,988k 1,222,103k p=0.173 n=6
Parse Time 8.10s (± 0.48%) 8.09s (± 0.48%) ~ 8.05s 8.15s p=0.935 n=6
Bind Time 2.25s (± 0.54%) 2.24s (± 0.66%) ~ 2.22s 2.25s p=0.410 n=6
Check Time 36.53s (± 0.28%) 36.37s (± 0.52%) ~ 36.17s 36.65s p=0.128 n=6
Emit Time 17.45s (± 0.42%) 17.44s (± 0.35%) ~ 17.33s 17.50s p=0.936 n=6
Total Time 64.33s (± 0.18%) 64.14s (± 0.24%) ~ 63.93s 64.39s p=0.093 n=6
mui-docs - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 1,961,349 1,961,349 ~ ~ ~ p=1.000 n=6
Types 696,910 696,910 ~ ~ ~ p=1.000 n=6
Memory used 1,778,045k (± 0.00%) 1,778,084k (± 0.00%) ~ 1,778,043k 1,778,123k p=0.148 n=6
Parse Time 9.88s (± 0.58%) 9.87s (± 0.20%) ~ 9.84s 9.89s p=0.418 n=6
Bind Time 3.38s (± 1.24%) 3.37s (± 0.56%) ~ 3.33s 3.38s p=0.572 n=6
Check Time 82.95s (± 0.44%) 82.66s (± 0.51%) ~ 82.02s 83.18s p=0.423 n=6
Emit Time 0.20s (± 2.02%) 0.20s (± 2.02%) ~ 0.20s 0.21s p=1.000 n=6
Total Time 96.42s (± 0.36%) 96.10s (± 0.47%) ~ 95.39s 96.63s p=0.298 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,221,120 1,221,139 +19 (+ 0.00%) ~ ~ p=0.001 n=6
Types 259,503 259,515 +12 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 2,387,138k (± 3.24%) 2,362,143k (± 2.58%) ~ 2,337,082k 2,486,910k p=0.810 n=6
Parse Time 5.97s (± 0.85%) 6.06s (± 1.16%) +0.09s (+ 1.48%) 5.97s 6.17s p=0.030 n=6
Bind Time 2.25s (± 1.12%) 2.24s (± 0.46%) ~ 2.23s 2.26s p=0.575 n=6
Check Time 39.81s (± 0.46%) 39.69s (± 0.26%) ~ 39.56s 39.83s p=0.173 n=6
Emit Time 3.16s (± 2.18%) 3.22s (± 1.23%) ~ 3.16s 3.28s p=0.149 n=6
Total Time 51.20s (± 0.42%) 51.21s (± 0.19%) ~ 51.09s 51.36s p=0.809 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,221,120 1,221,139 +19 (+ 0.00%) ~ ~ p=0.001 n=6
Types 259,503 259,515 +12 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 2,412,241k (± 0.02%) 2,413,032k (± 0.04%) ~ 2,411,683k 2,413,903k p=0.173 n=6
Parse Time 6.32s (± 1.08%) 6.25s (± 1.22%) ~ 6.13s 6.36s p=0.230 n=6
Bind Time 2.04s (± 0.67%) 2.05s (± 0.96%) ~ 2.02s 2.08s p=0.125 n=6
Check Time 40.20s (± 0.35%) 40.23s (± 0.24%) ~ 40.07s 40.34s p=0.575 n=6
Emit Time 3.21s (± 2.98%) 3.15s (± 1.71%) ~ 3.09s 3.22s p=0.298 n=6
Total Time 51.76s (± 0.47%) 51.69s (± 0.07%) ~ 51.63s 51.73s p=0.378 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 256,716 256,716 ~ ~ ~ p=1.000 n=6
Types 104,580 104,580 ~ ~ ~ p=1.000 n=6
Memory used 425,916k (± 0.01%) 425,888k (± 0.02%) ~ 425,764k 425,969k p=0.873 n=6
Parse Time 4.17s (± 0.67%) 4.17s (± 0.50%) ~ 4.15s 4.20s p=0.628 n=6
Bind Time 1.61s (± 1.07%) 1.61s (± 0.52%) ~ 1.61s 1.63s p=0.742 n=6
Check Time 22.18s (± 0.20%) 22.17s (± 0.28%) ~ 22.11s 22.28s p=0.520 n=6
Emit Time 1.71s (± 1.15%) 1.71s (± 1.59%) ~ 1.67s 1.75s p=0.625 n=6
Total Time 29.67s (± 0.19%) 29.66s (± 0.25%) ~ 29.61s 29.81s p=0.747 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 35 35 ~ ~ ~ p=1.000 n=6
Symbols 224,575 224,575 ~ ~ ~ p=1.000 n=6
Types 93,785 93,785 ~ ~ ~ p=1.000 n=6
Memory used 369,867k (± 0.03%) 369,801k (± 0.02%) ~ 369,718k 369,865k p=0.688 n=6
Parse Time 3.52s (± 0.86%) 3.49s (± 0.86%) ~ 3.46s 3.53s p=0.260 n=6
Bind Time 1.93s (± 1.08%) 1.94s (± 0.77%) ~ 1.92s 1.96s p=0.139 n=6
Check Time 19.29s (± 0.17%) 19.35s (± 0.27%) ~ 19.29s 19.43s p=0.064 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.73s (± 0.23%) 24.78s (± 0.30%) ~ 24.69s 24.92s p=0.167 n=6
vscode - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,822,274 2,822,274 ~ ~ ~ p=1.000 n=6
Types 957,520 957,520 ~ ~ ~ p=1.000 n=6
Memory used 2,994,876k (± 0.00%) 2,994,871k (± 0.00%) ~ 2,994,807k 2,994,968k p=0.471 n=6
Parse Time 13.84s (± 0.28%) 13.83s (± 0.14%) ~ 13.81s 13.86s p=0.416 n=6
Bind Time 4.15s (± 0.26%) 4.14s (± 0.22%) ~ 4.13s 4.15s p=0.149 n=6
Check Time 73.30s (± 0.24%) 73.37s (± 0.42%) ~ 73.11s 73.96s p=1.000 n=6
Emit Time 23.54s (± 0.69%) 23.56s (± 0.49%) ~ 23.45s 23.73s p=0.936 n=6
Total Time 114.83s (± 0.29%) 114.90s (± 0.25%) ~ 114.57s 115.40s p=0.630 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 265,866 265,866 ~ ~ ~ p=1.000 n=6
Types 108,401 108,401 ~ ~ ~ p=1.000 n=6
Memory used 410,574k (± 0.01%) 410,637k (± 0.02%) ~ 410,512k 410,760k p=0.298 n=6
Parse Time 3.21s (± 0.41%) 3.20s (± 0.46%) ~ 3.18s 3.22s p=0.617 n=6
Bind Time 1.41s (± 0.97%) 1.39s (± 0.75%) ~ 1.38s 1.41s p=0.161 n=6
Check Time 14.37s (± 0.43%) 14.44s (± 0.33%) ~ 14.38s 14.51s p=0.064 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 18.98s (± 0.29%) 19.03s (± 0.22%) ~ 18.99s 19.09s p=0.107 n=6
xstate-main - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 524,639 524,639 ~ ~ ~ p=1.000 n=6
Types 178,906 178,906 ~ ~ ~ p=1.000 n=6
Memory used 462,741k (± 0.01%) 462,651k (± 0.01%) -89k (- 0.02%) 462,570k 462,716k p=0.020 n=6
Parse Time 2.62s (± 0.57%) 2.61s (± 0.75%) ~ 2.58s 2.64s p=0.315 n=6
Bind Time 1.00s (± 0.84%) 0.98s (± 1.06%) -0.02s (- 1.84%) 0.96s 0.99s p=0.016 n=6
Check Time 15.32s (± 0.36%) 15.35s (± 0.67%) ~ 15.22s 15.52s p=0.810 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 18.93s (± 0.33%) 18.93s (± 0.43%) ~ 18.82s 19.06s p=1.000 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)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,761ms (± 0.36%) 2,876ms (± 9.58%) ~ 2,743ms 3,438ms p=0.374 n=6
Req 2 - geterr 6,159ms (± 0.62%) 6,587ms (± 9.87%) ~ 6,103ms 7,431ms p=0.378 n=6
Req 3 - references 348ms (± 1.64%) 349ms (± 1.61%) ~ 343ms 355ms p=0.746 n=6
Req 4 - navto 279ms (± 0.44%) 279ms (± 0.54%) ~ 277ms 281ms p=0.931 n=6
Req 5 - completionInfo count 1,357 1,357 ~ ~ ~ p=1.000 n=6
Req 5 - completionInfo 96ms (± 9.29%) 100ms (±11.90%) ~ 92ms 115ms p=0.774 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,428ms (± 0.45%) 2,442ms (± 0.60%) +14ms (+ 0.58%) 2,432ms 2,471ms p=0.045 n=6
Req 2 - geterr 3,868ms (± 0.25%) 3,865ms (± 0.12%) ~ 3,859ms 3,871ms p=0.377 n=6
Req 3 - references 297ms (± 0.55%) 297ms (± 0.41%) ~ 296ms 299ms p=0.357 n=6
Req 4 - navto 228ms (± 0.24%) 228ms (± 1.06%) ~ 226ms 233ms p=0.663 n=6
Req 5 - completionInfo count 1,519 1,519 ~ ~ ~ p=1.000 n=6
Req 5 - completionInfo 75ms (± 5.99%) 79ms (± 8.61%) ~ 68ms 84ms p=0.389 n=6
xstate-main-1-tsserver - node (v18.15.0, x64)
Req 1 - updateOpen 5,121ms (± 0.28%) 5,116ms (± 0.22%) ~ 5,100ms 5,130ms p=0.471 n=6
Req 2 - geterr 1,120ms (± 1.61%) 1,130ms (± 1.87%) ~ 1,096ms 1,147ms p=0.378 n=6
Req 3 - references 88ms (± 3.29%) 90ms (± 3.46%) ~ 84ms 93ms p=0.170 n=6
Req 4 - navto 449ms (± 3.62%) 452ms (± 0.46%) ~ 450ms 456ms p=0.809 n=6
Req 5 - completionInfo count 3,413 3,413 ~ ~ ~ p=1.000 n=6
Req 5 - completionInfo 861ms (± 2.37%) 875ms (± 1.31%) ~ 860ms 891ms p=0.199 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstate-main-1-tsserver - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 156.72ms (± 0.16%) 156.55ms (± 0.15%) -0.16ms (- 0.11%) 155.65ms 158.54ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 240.69ms (± 0.14%) 240.66ms (± 0.14%) ~ 238.96ms 245.79ms p=0.617 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 237.05ms (± 0.18%) 237.01ms (± 0.17%) ~ 235.42ms 244.62ms p=0.336 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 236.14ms (± 0.16%) 236.01ms (± 0.16%) -0.12ms (- 0.05%) 234.57ms 240.88ms p=0.002 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@sheetalkamat sheetalkamat changed the title [wip] Dont lower case type ref Do not lowercase typeReference directive name May 13, 2024
@typescript-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 May 13, 2024
@sheetalkamat sheetalkamat marked this pull request as ready for review May 13, 2024 23:07
@typescript-bot
Copy link
Collaborator

@sheetalkamat Here are the results of running the top 400 repos comparing main and refs/pull/58525/merge:

Everything looks good!

@@ -117,7 +166,6 @@ Info seq [hh:mm:ss:mss] Files (4)
Matched by default include pattern '**/*'
../lib/@types/UpperCasePackage/index.d.ts
Entry point for implicit type library 'UpperCasePackage'
Type library referenced via 'UpperCasePackage' from file '../lib/@app/lib/index.d.ts'
Copy link
Member Author

Choose a reason for hiding this comment

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

This stops showing as this because of how this is processed. When the UpperCasePackage typings are already found and are primary then they are not processed again and hence the file reason for this is never added. Thats a separate issue and am looking into updating that code as well but its not related to this change.

it just stopped showing because before this the package name by auto type ref was "UpperCasePackage" while by type Ref was "uppercasepackage" which made two distinct entries and hence processed again.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

This seems correct, but it also seems like a lot of people in the linked issue are misusing type reference directives. They’re not really supposed to resolve to files, right? They should be package directories in typeRoots?

@sheetalkamat
Copy link
Member Author

Yes they are but given that we dont disallow relative names in the type it just seems like something we shouldnt do. I mean we shouldnt be in buisness of "lowercasing". Please write what you mean. So i feel like this is better change.

We started doing this because of "Jquery.min" etc files and some other auto type Aquisition issues. But if you write "types" in your file, write it with correct casing is logical i think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lowercase filename in triple slash directives
3 participants