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

Revert "Proposed expandable hover API" #61132

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

gabritto
Copy link
Member

@gabritto gabritto commented Feb 6, 2025

Reverts #59940.

We merged it just to enable internal testing of the prototype implementation, so I'm reverting this for 5.8 RC, and a polished version should go in for 5.9.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 6, 2025
@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary.

@typescript-bot
Copy link
Collaborator

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.

@gabritto
Copy link
Member Author

gabritto commented Feb 7, 2025

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 7, 2025

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

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

@typescript-bot
Copy link
Collaborator

@gabritto Here are the results of running the user tests with tsc comparing main and refs/pull/61132/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@gabritto
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 34 34 ~ ~ ~ p=1.000 n=6
Symbols 62,390 62,390 ~ ~ ~ p=1.000 n=6
Types 50,395 50,395 ~ ~ ~ p=1.000 n=6
Memory used 194,950k (± 1.02%) 193,762k (± 0.81%) ~ 193,107k 196,959k p=0.297 n=6
Parse Time 1.32s (± 1.03%) 1.31s (± 0.84%) ~ 1.30s 1.32s p=0.090 n=6
Bind Time 0.73s 0.73s (± 0.70%) ~ 0.73s 0.74s p=0.174 n=6
Check Time 9.78s (± 0.14%) 9.81s (± 0.29%) ~ 9.77s 9.84s p=0.187 n=6
Emit Time 2.75s (± 0.65%) 2.76s (± 0.85%) ~ 2.73s 2.80s p=0.623 n=6
Total Time 14.59s (± 0.20%) 14.61s (± 0.31%) ~ 14.53s 14.65s p=0.335 n=6
angular-1 - node (v18.15.0, x64)
Errors 37 37 ~ ~ ~ p=1.000 n=6
Symbols 948,488 948,488 ~ ~ ~ p=1.000 n=6
Types 411,006 411,006 ~ ~ ~ p=1.000 n=6
Memory used 1,225,422k (± 0.00%) 1,225,425k (± 0.00%) ~ 1,225,366k 1,225,523k p=0.810 n=6
Parse Time 6.69s (± 0.93%) 6.73s (± 0.55%) ~ 6.65s 6.75s p=0.183 n=6
Bind Time 1.93s (± 0.71%) 1.93s (± 0.53%) ~ 1.92s 1.95s p=0.557 n=6
Check Time 32.16s (± 0.22%) 32.12s (± 0.46%) ~ 31.98s 32.38s p=0.294 n=6
Emit Time 15.32s (± 0.85%) 15.40s (± 0.36%) ~ 15.35s 15.49s p=0.148 n=6
Total Time 56.09s (± 0.34%) 56.17s (± 0.29%) ~ 55.93s 56.41s p=0.336 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,371,940 2,371,940 ~ ~ ~ p=1.000 n=6
Types 846,141 846,141 ~ ~ ~ p=1.000 n=6
Memory used 2,138,901k (± 0.00%) 2,138,924k (± 0.00%) ~ 2,138,841k 2,139,057k p=0.810 n=6
Parse Time 7.31s (± 0.17%) 7.31s (± 0.16%) ~ 7.29s 7.32s p=0.504 n=6
Bind Time 2.49s (± 0.39%) 2.49s (± 0.57%) ~ 2.47s 2.51s p=0.869 n=6
Check Time 73.10s (± 0.46%) 72.93s (± 1.17%) ~ 71.29s 73.77s p=1.000 n=6
Emit Time 0.14s (± 3.60%) 0.15s (± 2.75%) ~ 0.14s 0.15s p=0.112 n=6
Total Time 83.05s (± 0.40%) 82.88s (± 1.02%) ~ 81.26s 83.71s p=1.000 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,229,397 1,228,747 -650 (- 0.05%) ~ ~ p=0.001 n=6
Types 267,085 267,002 -83 (- 0.03%) ~ ~ p=0.001 n=6
Memory used 2,362,138k (± 0.01%) 2,361,149k (± 0.02%) -990k (- 0.04%) 2,360,332k 2,361,938k p=0.013 n=6
Parse Time 5.24s (± 0.57%) 5.23s (± 0.61%) ~ 5.18s 5.26s p=0.748 n=6
Bind Time 1.81s (± 0.75%) 1.82s (± 0.85%) ~ 1.79s 1.83s p=0.193 n=6
Check Time 35.46s (± 0.24%) 35.48s (± 0.42%) ~ 35.31s 35.64s p=0.810 n=6
Emit Time 3.04s (± 1.11%) 3.01s (± 1.06%) ~ 2.95s 3.04s p=0.223 n=6
Total Time 45.57s (± 0.20%) 45.55s (± 0.32%) ~ 45.40s 45.74s p=0.810 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,229,397 1,228,747 -650 (- 0.05%) ~ ~ p=0.001 n=6
Types 267,085 267,002 -83 (- 0.03%) ~ ~ p=0.001 n=6
Memory used 2,917,309k (±12.89%) 3,158,807k (± 0.02%) ~ 3,158,248k 3,159,697k p=0.471 n=6
Parse Time 6.91s (± 2.15%) 7.02s (± 0.75%) ~ 6.98s 7.12s p=0.199 n=6
Bind Time 2.14s (± 1.24%) 2.13s (± 0.97%) ~ 2.10s 2.15s p=0.257 n=6
Check Time 42.88s (± 0.40%) 42.93s (± 0.19%) ~ 42.78s 43.00s p=0.575 n=6
Emit Time 3.56s (± 2.45%) 3.44s (± 2.66%) ~ 3.33s 3.54s p=0.054 n=6
Total Time 55.50s (± 0.47%) 55.52s (± 0.32%) ~ 55.23s 55.78s p=0.936 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 263,074 263,031 -43 (- 0.02%) ~ ~ p=0.001 n=6
Types 106,881 106,875 -6 (- 0.01%) ~ ~ p=0.001 n=6
Memory used 441,319k (± 0.02%) 441,252k (± 0.03%) ~ 441,129k 441,440k p=0.128 n=6
Parse Time 3.56s (± 0.95%) 3.55s (± 1.25%) ~ 3.49s 3.61s p=0.936 n=6
Bind Time 1.32s (± 0.75%) 1.32s (± 0.89%) ~ 1.30s 1.33s p=1.000 n=6
Check Time 19.04s (± 0.38%) 18.98s (± 0.40%) ~ 18.90s 19.11s p=0.199 n=6
Emit Time 1.53s (± 0.92%) 1.52s (± 0.87%) ~ 1.50s 1.53s p=0.392 n=6
Total Time 25.45s (± 0.30%) 25.37s (± 0.46%) ~ 25.24s 25.55s p=0.229 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 70 70 ~ ~ ~ p=1.000 n=6
Symbols 226,113 226,113 ~ ~ ~ p=1.000 n=6
Types 94,488 94,488 ~ ~ ~ p=1.000 n=6
Memory used 371,860k (± 0.05%) 371,780k (± 0.04%) ~ 371,669k 372,058k p=0.689 n=6
Parse Time 2.89s (± 0.89%) 2.87s (± 0.60%) ~ 2.85s 2.89s p=0.373 n=6
Bind Time 1.58s (± 0.53%) 1.59s (± 1.10%) ~ 1.56s 1.61s p=0.933 n=6
Check Time 16.38s (± 0.23%) 16.42s (± 0.33%) ~ 16.37s 16.51s p=0.147 n=6
Emit Time 0.00s 0.00s (±244.70%) ~ 0.00s 0.01s p=0.405 n=6
Total Time 20.85s (± 0.30%) 20.88s (± 0.33%) ~ 20.81s 20.99s p=0.572 n=6
vscode - node (v18.15.0, x64)
Errors 3 3 ~ ~ ~ p=1.000 n=6
Symbols 3,280,686 3,280,686 ~ ~ ~ p=1.000 n=6
Types 1,128,284 1,128,284 ~ ~ ~ p=1.000 n=6
Memory used 3,354,740k (± 0.01%) 3,354,738k (± 0.01%) ~ 3,354,302k 3,355,057k p=0.936 n=6
Parse Time 14.60s (± 0.47%) 14.63s (± 0.56%) ~ 14.51s 14.71s p=0.470 n=6
Bind Time 4.64s (± 0.60%) 4.62s (± 0.42%) ~ 4.60s 4.65s p=0.332 n=6
Check Time 91.52s (± 3.18%) 89.42s (± 1.84%) ~ 88.51s 92.76s p=0.066 n=6
Emit Time 27.52s (± 8.52%) 28.02s (± 2.00%) ~ 27.65s 29.11s p=0.936 n=6
Total Time 138.28s (± 1.79%) 136.69s (± 1.25%) ~ 135.36s 139.84s p=0.199 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 293,378 293,378 ~ ~ ~ p=1.000 n=6
Types 119,581 119,581 ~ ~ ~ p=1.000 n=6
Memory used 447,128k (± 0.02%) 447,142k (± 0.02%) ~ 447,022k 447,323k p=0.810 n=6
Parse Time 4.11s (± 0.41%) 4.10s (± 0.73%) ~ 4.06s 4.14s p=0.744 n=6
Bind Time 1.78s (± 1.61%) 1.76s (± 0.85%) ~ 1.75s 1.79s p=0.510 n=6
Check Time 18.76s (± 0.56%) 18.82s (± 0.61%) ~ 18.67s 18.95s p=0.628 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.65s (± 0.40%) 24.68s (± 0.41%) ~ 24.55s 24.82s p=0.629 n=6
xstate-main - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 555,375 555,375 ~ ~ ~ p=1.000 n=6
Types 186,146 186,146 ~ ~ ~ p=1.000 n=6
Memory used 494,532k (± 0.01%) 494,507k (± 0.03%) ~ 494,225k 494,712k p=1.000 n=6
Parse Time 3.40s (± 0.58%) 3.39s (± 0.87%) ~ 3.37s 3.45s p=0.277 n=6
Bind Time 1.18s (± 1.12%) 1.19s (± 1.56%) ~ 1.18s 1.23s p=0.241 n=6
Check Time 19.57s (± 0.18%) 19.63s (± 1.11%) ~ 19.45s 20.04s p=0.748 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.16s (± 0.22%) 24.22s (± 0.87%) ~ 24.04s 24.61s p=0.872 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

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@gabritto Here are the results of running the top 400 repos with tsc comparing main and refs/pull/61132/merge:

Everything looks good!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Does VS Code need to know to not send this request anymore, or will it "just work" because the extra data will be ignored?

@gabritto
Copy link
Member Author

gabritto commented Feb 7, 2025

Does VS Code need to know to not send this request anymore, or will it "just work" because the extra data will be ignored?

Good question, I'll double check.

@mjbvz
Copy link
Contributor

mjbvz commented Feb 7, 2025

@gabritto On the VS Code side we gate our check to TS 5.7 , not specific pre-release version (plus you need the setting enabled). If you're planning to remove this, can you update VS Code to only send this request on TS 5.9+

@gabritto
Copy link
Member Author

gabritto commented Feb 7, 2025

@gabritto On the VS Code side we gate our check to TS 5.7 , not specific pre-release version (plus you need the setting enabled). If you're planning to remove this, can you update VS Code to only send this request on TS 5.9+

@mjbvz should I temporarily remove the whole feature from the vscode side as well? I could update it to only send the request for TS 5.9+, but in theory, we'll ship TS 5.9 nightly versions starting next week, and I won't bring this back into TSServer time for that. On the same vein, I'd also have to remove the setting.

@gabritto
Copy link
Member Author

gabritto commented Feb 7, 2025

Does VS Code need to know to not send this request anymore, or will it "just work" because the extra data will be ignored?

Good question, I'll double check.

I checked, and it works.

@gabritto gabritto merged commit 34ea32f into main Feb 7, 2025
32 checks passed
@gabritto gabritto deleted the revert-59940-gabritto/issue59029 branch February 7, 2025 20:44
@mylesmmurphy
Copy link

Hey @gabritto! Really great work on this, very excited to see this coming. I have some feedback / questions related to this that I mentioned here, and felt it might be relevant to your work and would love your feedback. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants