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

Go to References - Where Imports Used #1491

Merged
merged 18 commits into from May 30, 2022

Conversation

Jojoshua
Copy link
Contributor

Initial draft for #1485

Copy link
Contributor Author

@Jojoshua Jojoshua left a comment

Choose a reason for hiding this comment

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

I added a better dummy handler for this but every time I load the debugger I see Error: Language client is not ready yet error in the debug console. Any thoughts on what might be wrong?

@Jojoshua
Copy link
Contributor Author

I added a better dummy handler for this but every time I load the debugger I see Error: Language client is not ready yet error in the debug console. Any thoughts on what might be wrong?

FYI @jasonlyu123 here is the error..

Loading server from [....]\svelte-language-tools\packages\language-server\bin\server.js
packages/svelte-vscode/src/extension.ts:97
rejected promise not handled within 1 second: Error: Connection to server got closed. Server will not be restarted.

I notice that I do not have a server.js file where it is referencing(only a server.ts file). I did a yarn build but that did not create it or I cant find it. Is there a step I am missing to get this setup properly?

@Jojoshua
Copy link
Contributor Author

I added a better dummy handler for this but every time I load the debugger I see Error: Language client is not ready yet error in the debug console. Any thoughts on what might be wrong?

FYI @jasonlyu123 here is the error..

Loading server from [....]\svelte-language-tools\packages\language-server\bin\server.js packages/svelte-vscode/src/extension.ts:97 rejected promise not handled within 1 second: Error: Connection to server got closed. Server will not be restarted.

I notice that I do not have a server.js file where it is referencing(only a server.ts file). I did a yarn build but that did not create it or I cant find it. Is there a step I am missing to get this setup properly?

@jasonlyu123 I found the issue with this. I had 2 clones and did npm install instead of yarn install. I am not getting this error anymore

Copy link
Contributor Author

@Jojoshua Jojoshua left a comment

Choose a reason for hiding this comment

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

@dummdidumm I merged master and noticed this parsing issue. Could you verify this is OK

@Jojoshua
Copy link
Contributor Author

Added command to menu inside editor as well
image

@Jojoshua
Copy link
Contributor Author

This PR now fulfills #1485 and ready for another review. I was pleasantly surprised to see it works with dynamic imports references as well.

@Jojoshua
Copy link
Contributor Author

Jojoshua commented May 29, 2022

@dummdidumm Could you try the build again, I wasn't setup to format with prettier before...as a sidenote, should there be a vscode workspace setting for these packages so that prettier is automatically invoked when saving?

Copy link
Member

@dummdidumm dummdidumm 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 great, thank you! Only thing that's missing is a test. You'd create a new test file in test/plugins/typescript/features with setup similar to for example FindReferencesProvider.test.ts and create two testfiles in the adjacent testfiles folder. That's all very cumbersome, so I can do this, too, when I have time.

@Jojoshua
Copy link
Contributor Author

This is great, thank you! Only thing that's missing is a test. You'd create a new test file in test/plugins/typescript/features with setup similar to for example FindReferencesProvider.test.ts and create two testfiles in the adjacent testfiles folder. That's all very cumbersome, so I can do this, too, when I have time.

Thanks for the initial tip for this. I added 0aded87 which I would expect to work but I am getting no results from the provider.fileReferences call. Ideas?

@Jojoshua Jojoshua requested a review from dummdidumm May 30, 2022 03:44
@Jojoshua
Copy link
Contributor Author

@jasonlyu123 @dummdidumm I think this is ready for another look. My new test is passing but just FYI there is an unrelated test CompletionProvider.test.ts which is failing locally

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Great work, thank you very much!
Out of curiosity: How easy was it for you to contribute / navigate around the code base? Did you notice things would you improve?

@dummdidumm dummdidumm merged commit 5a6eba1 into sveltejs:master May 30, 2022
@Jojoshua
Copy link
Contributor Author

Jojoshua commented May 30, 2022

Great work, thank you very much! Out of curiosity: How easy was it for you to contribute / navigate around the code base? Did you notice things would you improve?

Thank you @dummdidumm, and I like that you asked this question.

This was very difficult for me to work through, not primarily due to the code base but rather that this domain (tooling for vscode/typescript) is all new to me. The concepts and architecture of how all this works together is complicated. I found a mermaid diagram for the volar project which helped reinforce what @jasonlyu123 was trying to explain to me throughout. https://github.com/johnsoncodehk/volar#high-level-system-overview Possibly a similar diagram for this project's architecture would have helped but there is no perfect replacement for sharing knowledge 1 on 1. Good news is that I know a little bit more than I did two weeks ago. If I had to do it all over again, I wouldn't have as much difficulty.

As for the codebase, minor criticisms

  • There is a fair amount of duplicated code
  • Debugging didn't work (or wasn't obvious) in all the packages

Things that I would note as helpful in the codebase:

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.

None yet

3 participants