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

68 multiple entries on go to definition #70

Merged
merged 6 commits into from
Jan 30, 2023

Conversation

Viijay-Kr
Copy link
Owner

@Viijay-Kr Viijay-Kr commented Jan 27, 2023

Closes #68

@karlhorky Here is the first draft to help avoid the extra definitions results from declaration files

I m using a small typescript plugin to help this cause.

currently the option to override the definitions from declaration files can be done to settings Which is documented in the readme

Let me know what you think about this PR

…ugin added to clean up definitions from declaration files
@Viijay-Kr Viijay-Kr requested a review from karlhorky January 27, 2023 19:03
@Viijay-Kr Viijay-Kr force-pushed the 68-multiple-entries-on-go-to-definition branch from a33c8f5 to 82b9a0b Compare January 27, 2023 19:10
@Viijay-Kr Viijay-Kr force-pushed the 68-multiple-entries-on-go-to-definition branch from 82b9a0b to d317b47 Compare January 27, 2023 19:13
"scope": "window",
"description": "Add a declaration file to skip definition results from this file. Useful for skipping unnecessary defintions from boilder plate projects",
"default": [
"node_modules/vite/client.d.ts",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will these paths work at any level of the project?

Eg. In a monorepo / workspace project, the path would be:

<project root>/packages/package-name/node_modules/next/types/global.d.ts

Would be cool if these also worked out of the box without any additional configuration

Copy link
Owner Author

Choose a reason for hiding this comment

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

it should since the plugin does a vague/naive string comparison .

Copy link
Owner Author

@Viijay-Kr Viijay-Kr Jan 28, 2023

Choose a reason for hiding this comment

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

@karlhorky Added a mono repo setup in the latest commit to test this out . Tested both in vite and next app inside monorepo/packages and it works as I expected.

you can also test it if you'd like
Open the monorepo folder in debug mode. Open App.tsx inside examples/monorepo/packages/vite-app/src/App.tsx and try going to the definition of styles.logo. You'll see only the css module definition is provided :)

Copy link
Collaborator

@karlhorky karlhorky left a comment

Choose a reason for hiding this comment

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

Seems like a really cool approach!

I suggested a few changes and asked some questions

@Viijay-Kr
Copy link
Owner Author

Seems like a really cool approach!

I suggested a few changes and asked some questions

Thanks a lot for the comments @karlhorky . Really Appreciate your time and effort.

@Viijay-Kr Viijay-Kr force-pushed the 68-multiple-entries-on-go-to-definition branch from 735bd32 to 6fb4730 Compare January 28, 2023 17:00
@Viijay-Kr Viijay-Kr merged commit 19198f9 into main Jan 30, 2023
@Viijay-Kr Viijay-Kr deleted the 68-multiple-entries-on-go-to-definition branch January 30, 2023 18:24
@karlhorky
Copy link
Collaborator

karlhorky commented Feb 1, 2023

I tried this 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
Copy link
Collaborator

karlhorky commented Feb 1, 2023

It seems like in the monorepo example, the packages/next-app workspace is not configured in package.json, wonder if this is related:

"workspaces": [
"apps/*",
"packages/ui",
"packages/tsconfig",
"packages/eslint-config-custom"
],

Eg. could that mean that the node_modules/next/types/global.d.ts is not being hoisted to the root?

@karlhorky
Copy link
Collaborator

If I should open a new issue, let me know.

@Viijay-Kr
Copy link
Owner Author

@karlhorky I too tried it after the version was shipped . It did not work for me either. I suspect it could be something to do with the typescript version difference .

I did not have time to investigate this further.

Hope I will find some time to debug this.

@karlhorky
Copy link
Collaborator

karlhorky commented Feb 1, 2023

I'll reopen + edit the original issue so that we don't lose track of it.

@Viijay-Kr
Copy link
Owner Author

It seems like in the monorepo example, the packages/next-app workspace is not configured in package.json, wonder if this is related:

"workspaces": [
"apps/*",
"packages/ui",
"packages/tsconfig",
"packages/eslint-config-custom"
],

Eg. could that mean that the node_modules/next/types/global.d.ts is not being hoisted to the root?

This is not related .

I have removed the next app from packages in order for the package to have it own node_modules folder .

If you look at vite example it has its own node modules setup.So this helped to test the use case you mentioned

However removing the next app from the list of packages didn't make a difference since the monorepo was setup using turbo repo which provides a next boilerplate

@karlhorky
Copy link
Collaborator

I reopened #68 and added a comment: #68 (comment)

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.

Multiple Entries on Go to Definition
2 participants