clinyong / vscode-css-modules

https://marketplace.visualstudio.com/items?itemName=clinyong.vscode-css-modules
MIT License
144 stars 39 forks source link

Intellisense priority issues breaks Go to definition #63

Open habovh opened 2 years ago

habovh commented 2 years ago

Hi,

I've been using this extension really fine on JS projects. It's great and really useful!

However on TS projects, it seems this extension is "fighting" for priority against the TS Intellisense provider. Unfortunately, the TS provider trumps this extension, and when using "Go to definition", it leads to the very generic type definition used to make the compiler happy instead of the actual class in the style module file.

This is rather annoying.

I've never developed a language server extension, but I was wondering if it was possible to increase priority of the suggestions/sources provided by this extension, so that even in case Intellisense matches 2 different definitions, the "chosen" one will always be the one from the extension?

As I've tried to work this out, I discovered that enabling the extension after the JS/TS language features are initialised, then the Go to definition works great. However when I quit and reopen VSCode, the default definition is reverted back to the generic TS one.

Note: I tried all of this without any TS plugin, using VSCode's bundled TS runtime, and on a Next.js project.

iChenLei commented 2 years ago

Yep, my friend(same company employee) encounted the same issue, but I have no idea to fix or improve ux. My friend's scenario is:

declare module '*.module.css' {
  const content: { [className: string]: string }
  export default content; 
}

A possible workaround is

declare module '*.module.css' {
-  const content: { [className: string]: string }
+ const content any;
  export default content; 
}

And then TS will give up type Intellisense. ( I checked it on my macbook

habovh commented 2 years ago

Ooh that's a clever trick! Makes sense that Intellisense would favour anything over any. Thanks for this hack, I wish it wasn't required however.

The "priority" is not great either with autocomplete. It's quite weird the extension is not able to communicate to VSCode that its autocompletion are 100% relevant and should be placed before random stuff.

karlhorky commented 1 year ago

Also just encountered this in a Next.js project (which has CSS modules types in the global types that come along with it in node_modules/next/types/global.d.ts):

Screenshot 2023-01-05 at 23 08 59

The other definition from global.d.ts is not very useful, it's just the generic type definition, including a string. I guess this would not be a desirable destination to use Go to Definition with:

Screenshot 2023-01-06 at 12 10 06

Altering the global.d.ts types file in node_modules to use any does avoid this problem, but I think requiring users to edit files in node_modules is probably not a good solution.

Is there any API in VS Code to remove / invalidate some other definition from within the extension? Or maybe even just re-defining the types to use any in the types for the extension?

iChenLei commented 1 year ago

Is there any API in VS Code to remove / invalidate some other definition from within the extension?

@karlhorky I don't know, I can't found any useful info from the official api documentation for this topic.

karlhorky commented 1 year ago

Edit: my post below appears to be not entirely accurate, looks like there is indeed a way to edit the autocomplete definitions that appear: https://github.com/clinyong/vscode-css-modules/issues/63#issuecomment-1404821676


Hm, it seems like this may be "as designed", as mentioned here:

The extension system is designed to be cooperative and so that a single extension cannot mute results of other extensions. We have no plans of changing that. However, if you think that the TypeScript/JavaScript extension behaves incorrect for a given scenario that please file a new, separate issue with steps to reproduce. Thanks

However, maybe this falls into "behaves incorrect for a given scenario", and deserves a bug in TypeScript, similar to this change to the "Duplicate Definitions" issue here:

@jrieken @mjbvz do you have any guidance here? It would be great to allow users to have the capability of opting out of these extra definition entries when a better one exists. This would enable one-click Go to Definition, which seems like the UX that is desirable for users. Ideally automatically by the extension, but it would also be acceptable to have the user manually opt out of specific definitions in some files (as long as they do not have to edit the global.d.ts file in their node_modules, which does not seem like a tenable solution).

We could also just create an issue in microsoft/vscode or microsoft/TypeScript to get some official feedback.

karlhorky commented 1 year ago

@iChenLei another idea: since you're using languages.registerDefinitionProvider here:

https://github.com/clinyong/vscode-css-modules/blob/4fc1c2aac1bffe88380656d54b99d6ac13151265/src/extension.ts#L23-L26

...what do you think about trying to also declare the same declare module *.module.css/sass/scss types like in the Next.js global config? (eg. with any, as below) Maybe then your type would override this? (or maybe it doesn't work like I'm hoping?)

declare module '*.module.css' {
  const classes: any
  export default classes
}

declare module '*.module.sass' {
  const classes: any
  export default classes
}

declare module '*.module.scss' {
  const classes: any
  export default classes
}
zardoy commented 1 year ago

@karlhorky Only in case of TypeScript/JavaScript/Vue there is an API to take full control over definitions / completions and so on returned by TS language service with TS plugins. I've made an extension that removes generic definition: https://github.com/zardoy/typescript-vscode-plugins/ (enabled by default with miscDefinitionImprovement, source code).

I also noticed that defition priority is not consistent. With "editor.gotoLocation.multipleDefinitions": "gotoAndPeek" it was always revealing .css file definition first, but this week it started to focus on .d.ts file instead, I have no idea why, but because of this I made solution mentioned above.

karlhorky commented 1 year ago

~I'm using the React CSS modules extension (repo) at the moment, which appears to avoid the problem somehow... 🤔~

Edit: Actually sorry, this is incorrect - the issue is still present over there.

karlhorky commented 1 year ago

@zardoy I'm not sure what you mean by "TypeScript/JavaScript/Vue", but reading the source code of your extension TypeScript Essential Plugins (repo) it seems there may be code that could provide a way forward. Some interesting parts:

    proxy.getDefinitionAndBoundSpan = (fileName, position) => {
      const prior = info.languageService.getDefinitionAndBoundSpan(fileName, position)
      // ...
      prior.definitions = prior.definitions.filter(({ fileName, containerName, containerKind, kind, name, ...rest }) => {
        // ...
        if (moduleDeclaration?.name.text === '*.module.css') return false
        return true
      })
      // ...
    })

If I'm understanding correctly, this is filtering out previous definitions if they have the name *.module.css, which could also be extended to include *.module.scss and *.module.sass.

@iChenLei @clinyong what do you think about this approach?

zardoy commented 1 year ago

I'm not sure what you mean by "TypeScript/JavaScript/Vue"

@karlhorky I meant that it is possible to fully control language server features of these languages.

If I'm understanding correctly, this is filtering out previous definitions

Yes, the logic is quite simple, however I included additional pattern check to ensure we don't do extra work on every definition. And yes it can be extended, I just forgot to add .scss and sass extensions, will fix it.

Also feel free to ask whatever you want!

Stan-Stani commented 8 months ago

For me, the weird thing is even if I replace Next.js types for scss module imports with any I just get any as the hover type definition. But if I control click it takes me to the appropriate selector in styles.module.scss. Anyone have any tips to get hover definitions working?