TypeStrong / atom-typescript

The only TypeScript package you will ever need
https://atom.io/packages/atom-typescript
MIT License
1.13k stars 205 forks source link

[WIP] Add a new command of `checkRelatedFiles` #1531

Closed firejune closed 4 years ago

firejune commented 4 years ago
lierdakil commented 4 years ago

Hi. Thanks for creating the PR!

It would be really helpful if you could add a couple sentences describing the motivation behind the work you're doing and outline the general design you're implementing. Since the diff is on the larger side, it's not easy to just look at it and get a good mental grasp on what you're doing.

Here are some things I found questionable or unclear after a cursory look through the diff:

  1. I don't claim to completely understand the code for "related files check", not after one look, but it would seem that it will only find files "related" to current line, not currently active text editor.

  2. Why are you using semanticDiagnosticsSync and syntacticDiagnosticsSync? Those are from the synchronous API, which is generally to be avoided. You could as well trigger good old geterr, passing the list of "related files" to it, no?

  3. Re-exporting reportBusyWhile from TypescriptServiceClient does not look like a good idea. It makes the code more tightly interdependent, with little apparent benefit. Generally we try to keep coupling between components as loose as possible here, so please try to avoid threading dependencies through other dependencies.

firejune commented 4 years ago

Hi. Thanks for the feedback.

Yes, I understand. This PR is volume is too large. So I will close this PR soon. And I will recreating by divided into semantic units.

Then please review again.