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

refactor(lsp): clean up tsc requests #20743

Merged
merged 7 commits into from
Oct 2, 2023

Conversation

nayeemrmn
Copy link
Collaborator

We have two useless intermediate representation of requests between the TsServer::<method>() args and the typescript languageService[method]() args. Namely the RequestMethod enum and the 'serializations' returned by RequestMethod::to_value(), which still need to be parsed into the final TS API args. The latter also has type declarations in compiler.d.ts.

So there are 4 different places in our tsc abstraction layer that need to be changed if I wanted to, say, add a new workspace setting and pass that into TSC.

This reduces that to 1. Now things like the specifier denormalization and normalization are near each other, and located at each respective TsServer method.

@nayeemrmn nayeemrmn requested a review from dsherret September 30, 2023 10:16
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Looking good, but we should probably call the higher level functions of TsServer in the tests so that we're actually testing that struct.

@nayeemrmn nayeemrmn requested a review from dsherret October 1, 2023 05:14
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Nice cleanup!

@nayeemrmn nayeemrmn merged commit 6fd2d08 into denoland:main Oct 2, 2023
@nayeemrmn nayeemrmn deleted the lsp-tsc-specifier-cleanup branch October 2, 2023 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants