- Notifications
You must be signed in to change notification settings - Fork 13.2k
Revert "Proposed expandable hover API"#61132
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This reverts commit 80eeb4e.
typescript-bot commented Feb 6, 2025
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 commented Feb 6, 2025
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 commented Feb 7, 2025
@typescript-bot test it |
typescript-bot commented Feb 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot commented Feb 7, 2025
@gabritto Here are the results of running the user tests with tsc comparing Everything looks good! |
typescript-bot commented Feb 7, 2025
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
typescript-bot commented Feb 7, 2025
@gabritto Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Feb 7, 2025
@gabritto Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
jakebailey left a comment
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.
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 commented Feb 7, 2025
Good question, I'll double check. |
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 commented Feb 7, 2025
@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 commented Feb 7, 2025
I checked, and it works. |
34ea32f into mainUh oh!
There was an error while loading. Please reload this page.
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.