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

Use tsserver geterr command & events. reloadProjects for file renames #132

Merged
merged 1 commit into from
Jul 14, 2024

Conversation

benjreinhart
Copy link
Contributor

@benjreinhart benjreinhart commented Jul 14, 2024

Lots of trial and error but looks like using geterr command is better than the semanticDiagnosticSync command:

  1. It's more convenient since it accepts a list of files rather than one file
  2. It sometimes fires events unprompted and so we can listen to those as well as ones we ask it for
  3. Of the very little literature i've found on the internet about tsserver, I've seen this referenced

This PR also solves one annoying bug when renaming files

  1. Rename a a.ts that is imported by b.ts to c.ts
  2. Semantic diagnostics report an error in b.ts that a.ts doesn't exist
  3. Great, all works so far.
  4. Rename c.ts back to a.ts.
  5. Semantic diagnostics still report an error in b.ts that a.ts doesn't exist.
  6. This is wrong, a.ts does exist.

This PR issues the reloadProjects command which gets it back to a working state.

I also added more documentation.

@benjreinhart benjreinhart requested a review from nichochar July 14, 2024 01:26
@benjreinhart benjreinhart merged commit 4c7c4fc into main Jul 14, 2024
1 check passed
@benjreinhart benjreinhart deleted the tsserver-updates branch July 14, 2024 20:20
Copy link
Contributor

@nichochar nichochar left a comment

Choose a reason for hiding this comment

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

This is really interesting, thanks for the thorough documentation. Sounds like a blog post on working with tsserver would be really high value ?

// TODO: Can we be smarter and only do this for cells that import
// this cell, rather than all cells?
sendAllTypeScriptDiagnostics(tsserver, session);
// TODO: Given the amount of differences here and elsewhere when renaming cells,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, I was thinking the same thing

// NOTE: reloading the project sends diagnostic events without calling `geterr`.
// However, it seems to take a while for the diagnostics to be sent, so we still
// request it below.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

nanonit: extra line here

* 2. Response: A response from the server to a specific client request.
* 3. Event: An event from the server to the client.
*
* Request and responses are identified a unique number called `seq`. `seq` is incremented
Copy link
Contributor

Choose a reason for hiding this comment

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

identified by* a unique

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