language-tools icon indicating copy to clipboard operation
language-tools copied to clipboard

Assist with store imports with svelte $ prefix

Open Jojoshua opened this issue 3 years ago • 5 comments

Description

Regarding the shortcut to directly access store values , https://svelte.dev/docs#component-format-script-4-prefix-stores-with-$-to-access-their-values

A pattern I've noticed which has become a sore point over and over is refactoring or writing new code which references a store with the $ prefix. For example, I might write or copy from one component to another...

{#if $showTheThing}

{/if}

However the import is missing so I will click somewhere on $showTheThing to do CTRL + . so that VSCode can import this from the store. This doesn't provide the hint I want. All I get shown is "(svelte) Disable Missing Declaration for this line"

So then I remove the $ from the string, then do CTRL + . again to import,

{#if showTheThing}

{/if}

Then go back to the beginning of the variable to prepend it back with $

{#if $showTheThing}

{/if}

Proposed solution

I would like to use CTRL + . on $showTheThing and have it suggest the correct import (or even multiple imports at once in the case that many imports are missing).

Alternatives

No response

Additional Information, eg. Screenshots

No response

Jojoshua avatar Aug 12 '22 14:08 Jojoshua

@jasonlyu123 @dummdidumm If you could point me in the right direction for this I can look into it

Jojoshua avatar Aug 25 '22 02:08 Jojoshua

I can only think of a hacky workaround for it. It's similar to a component import quick fix workaround we have. https://github.com/sveltejs/language-tools/blob/3e7c986c8596da013ef72b139512575cf08be4b2/packages/language-server/src/plugins/typescript/features/CodeActionsProvider.ts#L384

instead of triggering the completion at the real position, we trigger it at the start of the file. Trick Typescript to get global completions. And we resolve the one that matches the missing definition identifier.

I tried adding the logic to the completion but it seems to be too expensive for completion. I think we could only add it to code action if we couldn't find a more performant workaround.

Edit: Or maybe we could probably hide a variable without the prefix somewhere in svelte2tsx. And we can find the variable and trigger the code action there. But that might need some extra work to ensure it doesn't show up in reference, diagnostic rename etc.

jasonlyu123 avatar Aug 25 '22 02:08 jasonlyu123

Here's an idea to brainstorm...Would it be possible to sortof emulate the steps you have to manually do if we get an event that says "The user is looking for imports" so then...

  1. Check the start of the text range begins with $
  2. Remove the $, then trigger completions
  3. Add the $ back to the beginning of the text

Jojoshua avatar Aug 25 '22 03:08 Jojoshua

It would be possible but the performance would not be great either. This would cause TypeScript to rebuild twice.

jasonlyu123 avatar Aug 25 '22 03:08 jasonlyu123

Edit: Or maybe we could probably hide a variable without the prefix somewhere in svelte2tsx. And we can find the variable and trigger the code action there. But that might need some extra work to ensure it doesn't show up in reference, diagnostic rename etc.

Could this be done on-demand and then removed...or would it have the same negative perf?

Jojoshua avatar Aug 25 '22 03:08 Jojoshua

Could this be done on-demand and then removed...

On-demand would always have a performance issue. That's because TypeScript will analyse the whole project again when one file is updated. Vetur uses an on-demand method for component auto-import and the perf isn't great.

instead of triggering the completion at the real position, we trigger it at the start of the file. Trick Typescript to get global completions. And we resolve the one that matches the missing definition identifier.

I did find a way the slightly speed up this method using ts.lanagueService.getNavigateToItems. Though I am still not sure if it's good enough for completion. Are you interested in implementing it? If so I can give you some hints about it.

jasonlyu123 avatar Dec 06 '22 04:12 jasonlyu123

That's because TypeScript will analyse the whole project again when one file is updated.

Could there be a good reason for this though? If not, would the reference links not show up right?

This seems out of my element but I don't mind giving it a shot if given some support.

Jojoshua avatar Dec 06 '22 23:12 Jojoshua

Some background first. The completion is consist of two steps. The first one is the summary list. And when you focus on one entry. A second request will be sent to resolve the computationally heavy detail. And the method is to fake an entry to let TypeScript resolve it. It is a workaround for typescript not publicly exposing the auto import API.

We can repurpose this method to be a more general-purpose one https://github.com/sveltejs/language-tools/blob/37d73e9deefc716588d046116c8bf2f09fca3ca0/packages/language-server/src/plugins/typescript/features/CodeActionsProvider.ts#L520

The new step can be

  1. get sourceFile, program, typeChecker and quote preference
  2. loop through the diagnostics and the identifier that is undeclared
  3. If the identifier starts with $ go to another method/function for a store quick fix. If it's event handler go to handler quick fix.

The stores import quick-fix step

  1. Call ts.lanagueService.getNavigateToItems with the identifier name without the $ prefix. The maxResultCount can be some small number like 10 or 20. We can experiment with it to see what works better.
  2. Use the result to create some fake ts.ts.CompletionEntry
  3. Pass the fake completion entry to ts.languageService.getCompletionEntryDetails like what we did in component quick fix. Only the position parameter should be the start of the file(0) or the end of the file(sourceFile.getText().length). Not sure which one works better. And whether to use errorPreventingUserPreferences depends on whether is from a svelte file.
  4. convert the result back to ts.CodeFixAction

jasonlyu123 avatar Dec 08 '22 05:12 jasonlyu123

I started taking a look at this but I am observing something odd...the intended behavior already working (when I load my project from the language-tools extension debug mode).

Just to verify, I reinstalled the official extension again and it is back to NOT working. I'm not sure how to even test this now and why it would be working during the extension host mode only.

extension-host

Jojoshua avatar Dec 17 '22 06:12 Jojoshua

Guess your local build is still an old version. It did work before but we changed the way we transform the $store variable a while ago to fix some type-checking issues. You can run yarn bootstrap to update it.

jasonlyu123 avatar Dec 17 '22 07:12 jasonlyu123

Ok so I am now getting consistent behavior and seeing where createElementEventHandlerQuickFix is being activated with the quick fix shortcut.

However trying to test with what I expected would work via https://github.com/sveltejs/language-tools/pull/1731 however I am not seeing the event quick fix actually working.

Putting in <input on:keyup="{onKeyUp}" /> and doing quick fix on onKeyUp is not providing the fix on both the official plugin release and the extension host. Could this have broken recently? I am using TS 4.9.4 and vscode 1.74.1

Jojoshua avatar Dec 17 '22 20:12 Jojoshua

That seems to be because Typescript thinks that's possibly a typo for onkeyup. If you use handleKeyup it would show up. Not sure what can we do about it.

jasonlyu123 avatar Dec 19 '22 04:12 jasonlyu123

Alrighty, I just linked a PR for this, let me know what you think.

Jojoshua avatar Dec 19 '22 22:12 Jojoshua