react-ts-css icon indicating copy to clipboard operation
react-ts-css copied to clipboard

Multiple Entries on Go to Definition

Open karlhorky opened this issue 3 years ago • 14 comments

In the app/page.tsx file in the Next.js app dir example cmd-clicking on the styles.code property to trigger Go to Definition in VS Code shows an extra entry of the node_modules/next/types/global.d.ts file with the *.module.css type definition.

Screenshot 2023-01-26 at 12 22 52

This makes it no longer a one-click Go to Definition interaction. It would be great to get rid of these generic *.module.css / *.module.scss / *.module.sass entries so that it's a single click again.

This is also a problem with other extensions:

  • https://github.com/clinyong/vscode-css-modules/issues/63

Potential approach

It seems that @zardoy has found a potentially interesting approach in his extension TypeScript Essential Plugins (repo). Some interesting parts of the code:

    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
      })
      // ...
    })
  • https://github.com/zardoy/typescript-vscode-plugins/blob/a102a76786a0292aa8a9a46fb6cc43595e8871a7/typescript/src/definitions.ts#L7-L8
  • https://github.com/zardoy/typescript-vscode-plugins/blob/a102a76786a0292aa8a9a46fb6cc43595e8871a7/typescript/src/definitions.ts#L123
  • https://github.com/zardoy/typescript-vscode-plugins/blob/a102a76786a0292aa8a9a46fb6cc43595e8871a7/typescript/src/definitions.ts#L141

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.

karlhorky avatar Jan 26 '23 11:01 karlhorky

I tried the changes from #70 out just now in v1.6.2, but I'm still getting the duplicate entries:

Screenshot 2023-02-01 at 19 05 45

More details:

  • Page: packages/fusion.upleveled.io/app/(auth)/login/page.tsx
  • CSS Module File: packages/fusion.upleveled.io/app/(auth)/login/page.module.scss
  • Next.js type definitions: node_modules/next/types/global.d.ts

karlhorky avatar Feb 01 '23 18:02 karlhorky

@karlhorky Can u also let me know the typescript version you are using in your project ?

One thing I tried in the example monorepo project is changing the typescript version to point to the node module version.

In this case it works. Will continue to debug further

https://user-images.githubusercontent.com/8697234/216135976-f1a23c01-9760-4e76-ab6c-93c59b0bcf90.mov

Viijay-Kr avatar Feb 01 '23 18:02 Viijay-Kr

I tried with the following, neither seem to work (tried reloading the editor as well between switching):

  • VS Code's version: TypeScript 4.9.4
  • Workspace version: TypeScript 4.9.5

In the video above, there are two things that may be different:

  1. The Vite app is open
  2. VS Code seems to be using examples/monorepo/packages/vite-app as the root (in many situations when working in a monorepo, the VS Code root would be examples/monorepo)

karlhorky avatar Feb 01 '23 19:02 karlhorky

I tried with the following, neither seem to work (tried reloading the editor as well between switching):

  • VS Code's version: TypeScript 4.9.4
  • Workspace version: TypeScript 4.9.5

In the video above, there are two things that may be different:

  1. The Vite app is open
  2. VS Code seems to be using examples/monorepo/packages/vite-app as the root (in many situations when working in a monorepo, the VS Code root would be examples/monorepo)

@karlhorky That was my bad . However it doesn't make any difference. Even if I open the entire monorepo project I am able to make the feature work by swapping the typescript version.

I may have a potential fix which I would try . I will keep you posted

Viijay-Kr avatar Feb 01 '23 19:02 Viijay-Kr

Seems like it is reproducible in the examples/next-js-app-dir-css-modules example (nothing to do with monorepos / workspace packages):

Screenshot 2023-02-01 at 20 16 39

Change to this directory, install the dependencies and open VS Code using:

cd examples/next-js-app-dir-css-modules
npm install
code .
  1. Open app/page.tsx
  2. cmd/ctrl-click on the code in styles.code on line 13

karlhorky avatar Feb 01 '23 19:02 karlhorky

@karlhorky So I shipped 1.6.4 (latest) which fixes the problem.

As pointed out here https://github.com/Viijay-Kr/typescript-cleanup-defs/pull/1#issuecomment-1412634065

The extension was not shipped with node_modules which resulted in a error where plugin could not be loaded since the node_modules folder wasn't part of the installed extension.

Viijay-Kr avatar Feb 01 '23 19:02 Viijay-Kr

Indeed, 1.6.4 seems to be fixed, both with the built-in VS Code TypeScript version (4.9.4) and the Workspace version (4.9.5).

Thanks for this, nice work!

Closing.

karlhorky avatar Feb 01 '23 22:02 karlhorky

@Viijay-Kr Hmm... need to reopen this :( Looks like this Next.js entry removal is broken again in version 1.9.6 with *.module.scss imports (not sure which version was the first one with the broken behavior):

Screenshot 2023-03-05 at 13 22 21

Here's the project repo: https://github.com/upleveled/next-js-example-winter-2023-vienna-austria

Here's the config I tried:

.vscode/settings.json

{
  "reactTsScss.autoComplete": true,
  "reactTsScss.baseDir": "src",
  "reactTsScss.cleanUpDefs": [
    "*.module.css",
    "*.module.scss",
    "*.module.sass",
    "*.module.less",
    "*.module.styl",
    "node_modules/next/types/global.d.ts"
  ],
  "reactTsScss.codelens": false,
  "reactTsScss.cssAutoComplete": true,
  "reactTsScss.cssDefinitions": true,
  "reactTsScss.cssSyntaxColor": true,
  "reactTsScss.definition": true,
  "reactTsScss.diagnostics": true,
  "reactTsScss.peekProperties": true,
  "reactTsScss.references": false,
  "reactTsScss.tsconfig": "./tsconfig.json",
  "typescript.tsdk": "./node_modules/typescript/lib",
  "typescript.enablePromptUseWorkspaceTsdk": true,
}

karlhorky avatar Mar 05 '23 12:03 karlhorky

Ah to be clear, this only happens when cmd-clicking on the import path at the top (./layout.module.scss).

Maybe this never worked to cmd-click on the import path 🤔 Would be great to be able to get it also enabled to be 1-click if possible.

Using cmd-click on each CSS class still works as it did before 🙌 🎉

https://user-images.githubusercontent.com/1935696/222961056-4d3dab47-5ea2-42c8-b83d-e978565d3b7f.mp4

karlhorky avatar Mar 05 '23 12:03 karlhorky

Hey @karlhorky how is it going ? 👋🏼.

Maybe this never worked to cmd-click on the import path 🤔 Would be great to be able to get it also enabled to be 1-click if possible.

Yeah , this is true , the plugin never considers the import path as mentioned here by Zardoy.

Do you think its a bad experience ?

Viijay-Kr avatar Mar 05 '23 14:03 Viijay-Kr

I'm well thanks! You're good too I trust?

Which of @zardoy's comments in https://github.com/Viijay-Kr/typescript-cleanup-defs/pull/1 are you referring to? I see this comment which refers to a path, but is this actually related to a cmd-click on the path of the import?

Do you think its a bad experience ?

Since all other cmd-clicks work with a single click, I would expect that this one would also work this way for UX consistency.

karlhorky avatar Mar 05 '23 15:03 karlhorky

I'm specifically referring to the part where he mentions about neglecting useful definitions in the first comment he did

However , in this case , import statements definitely is not useful , so I guess imports are not handled gracefully.

I will try to find some time in the coming week, the work should be done in the plugin so if @zordoy is able to pitch in it would be great.

Viijay-Kr avatar Mar 05 '23 15:03 Viijay-Kr

If possible, I'll try to help next week as well

strlns avatar Mar 05 '23 15:03 strlns

@Viijay-Kr ha! I see you try to mention me in many wrong ways 😆

And do you mean you something like this? https://github.com/zardoy/typescript-vscode-plugins/blob/2cd6a12d4e651db69d176770023c2e5247ce85aa/typescript/src/definitions.ts#LL146C13-L156C15

as you see its just a few lines of code, shouldn't be hard...

zardoy avatar Mar 05 '23 18:03 zardoy