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

fix:consistent type imports warning #2647

Closed
wants to merge 2 commits into from
Closed

fix:consistent type imports warning #2647

wants to merge 2 commits into from

Conversation

harshmangalam
Copy link
Contributor

@harshmangalam harshmangalam commented Jan 15, 2023

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

fix consistent type import warning

@stackblitz
Copy link

stackblitz bot commented Jan 15, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@zanettin
Copy link
Contributor

Thanks @harshmangalam for taking the time to create this PR 🙏
May i ask what the motivation for the made changes is? As far as i know, we shouldn't use type imports when ever possible which covers with the statement of andrewbranch (member of the typescript team) as he stated here microsoft/TypeScript#39861 (comment).
Many thanks for a quick reply and for your help pushing qwik forward 🙏

@zanettin zanettin added the WAITING FOR: user Further information is requested from the issue / pr opener label Feb 16, 2023
@harshmangalam
Copy link
Contributor Author

Vs code warn when we import type without type keyword.

@zanettin
Copy link
Contributor

Wow thanks a lot for the quick reply 🔥 🙏

I just checked it on latest main branch on my side vscode doesn't complain about it on the imports 🙈
Bildschirmfoto 2023-02-16 um 21 10 51

tsc: 4.9.5

@harshmangalam
Copy link
Contributor Author

Your eslint is disabled hence you are not getting any warning.

@harshmangalam
Copy link
Contributor Author

entry ssr tsx - Untitled (Workspace) - Visual Studio Code 17_02_2023 21_59_13

Typescript v4.9.5

@zanettin
Copy link
Contributor

zanettin commented Feb 17, 2023

Thanks again for your feedback 🙏

My eslint is working and reports all issues as it should except the one you refer here 🙈
The mentioned rule @typescript-eslint/consistent-type-imports is not part of the recommended config of @typescript-eslint@5.52.0 which is used on the default generated .eslintrc.cjs nor in any other referenced configs/inline config sets.

these presets are defined and as far as i can see non of them have the rule included 🙈 (pls correct me if i am wrong 🙏 )

can it be, that you use a global version of eslint in vscode which uses another config file?
maybe we can get a step further when you do these steps on your app:

  • run pnpm lint (or npm / yarn) 😄
  • if the error is reported proceed, otherwise vscode uses another config
  • replace the lint script within the package.json file with "lint": "eslint src/**/*.ts* --debug"
  • search in the generated output for eslintrc:config-array-factory Loading JS config file and check if your root file is used
  • search also for consistent-type-imports

would be very interested to hear what the outcome of this is 🙏 thanks again for your help!

@harshmangalam
Copy link
Contributor Author

@zanettin got your points.

@zanettin
Copy link
Contributor

Hi @harshmangalam
Just wanted to ask if you already had the time to check, where this rule is coming from 👼 Would be very interested to hear what the outcome is. Thanks again 🙏

@harshmangalam
Copy link
Contributor Author

Hey I follow your process now everything is working fine.

@zanettin
Copy link
Contributor

So glad to hear 🙏 Thanks anyways for creating this PR and doing the debugging! hope to see soooonish another PR from you 😃

@zanettin zanettin closed this Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WAITING FOR: user Further information is requested from the issue / pr opener
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants