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

Multiple Entries on Go to Definition #68

Open
karlhorky opened this issue Jan 26, 2023 · 14 comments · Fixed by #70 or Viijay-Kr/typescript-cleanup-definitions#1
Open

Multiple Entries on Go to Definition #68

karlhorky opened this issue Jan 26, 2023 · 14 comments · Fixed by #70 or Viijay-Kr/typescript-cleanup-definitions#1

Comments

@karlhorky
Copy link
Collaborator

karlhorky commented Jan 26, 2023

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:

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
      })
      // ...
    })

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 karlhorky changed the title Multiple Entries on Jump to Definition Multiple Entries on Go to Definition Jan 26, 2023
@karlhorky karlhorky reopened this Feb 1, 2023
@karlhorky
Copy link
Collaborator Author

karlhorky commented Feb 1, 2023

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

@Viijay-Kr
Copy link
Owner

Viijay-Kr commented Feb 1, 2023

@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

Screen.Recording.2023-02-01.at.19.51.48.mov

@karlhorky
Copy link
Collaborator Author

karlhorky commented Feb 1, 2023

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)

@Viijay-Kr
Copy link
Owner

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

@karlhorky
Copy link
Collaborator Author

karlhorky commented Feb 1, 2023

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

@Viijay-Kr
Copy link
Owner

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

As pointed out here Viijay-Kr/typescript-cleanup-definitions#1 (comment)

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.

@karlhorky
Copy link
Collaborator Author

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
Copy link
Collaborator Author

karlhorky commented Mar 5, 2023

@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 karlhorky reopened this Mar 5, 2023
@karlhorky
Copy link
Collaborator Author

karlhorky commented Mar 5, 2023

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 🙌 🎉

Kapture.2023-03-05.at.13.41.07.mp4

@Viijay-Kr
Copy link
Owner

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 ?

@karlhorky
Copy link
Collaborator Author

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

Which of @zardoy's comments in Viijay-Kr/typescript-cleanup-definitions#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.

@Viijay-Kr
Copy link
Owner

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.

@strlns
Copy link
Collaborator

strlns commented Mar 5, 2023

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

@zardoy
Copy link

zardoy commented Mar 5, 2023

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants