Skip to content

chore: prefer type imports#5835

Merged
antfu merged 5 commits into
mainfrom
prefer-type-imports
Dec 19, 2021
Merged

chore: prefer type imports#5835
antfu merged 5 commits into
mainfrom
prefer-type-imports

Conversation

@Shinigami92
Copy link
Copy Markdown
Member

Description

Prefer type imports

Additional context

This forces to use always type imports. This improves TS compiled output and removes (in some cases) unnecessary empty imports.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p1-chore Doesn't change code behavior (priority) label Nov 25, 2021
@Shinigami92 Shinigami92 self-assigned this Nov 25, 2021
@Shinigami92 Shinigami92 marked this pull request as draft November 25, 2021 13:04
@Shinigami92 Shinigami92 marked this pull request as ready for review November 25, 2021 13:28
Comment thread packages/playground/react-emotion/vite.config.ts
@userquin
Copy link
Copy Markdown
Contributor

FYI: with typescript 4.5.* you can also use import { type XXX, YYY, ZZZ } from '....'

@Shinigami92
Copy link
Copy Markdown
Member Author

FYI: with typescript 4.5.* you can also use import { type XXX, YYY, ZZZ } from '....'

I know, but not sure right now if this is necessary + if the lint rule support it already.
The current PR works right now 🙂

@userquin
Copy link
Copy Markdown
Contributor

FYI: with typescript 4.5.* you can also use import { type XXX, YYY, ZZZ } from '....'

I know, but not sure right now if this is necessary + if the lint rule support it already. The current PR works right now 🙂

ok, here the link: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-rc/#type-on-import-names

patak-cat
patak-cat previously approved these changes Nov 25, 2021
import { resolveServerOptions } from './server'
import type { ResolvedPreviewOptions, PreviewOptions } from './preview'
import { resolvePreviewOptions } from './preview'
import type { CSSOptions } from './plugins/css'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the benefit of forcing this tho?

To me, it was mainly to avoid accidentally introducing side-effects from the modules that you only need the types from. But if you are already imported some runtime from a module, the type specifier kinda lost the advantage.

I guess the new syntax in 4.5 will be better, but currently, it breaks many imports from one line to two, which kinda makes the code harder to read and maintain to me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes you are right that the TS 4.5 syntax could help, BUT I would like to use eslint fixer for that

The two lines are sometimes better cause they are then not wrapped cause of printWidth
Also the imports at the top are mostly not for read but auto organized

When TS 4.5 syntax is supported by @typescript-eslint we can switch to the option

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@johnnyshankman
Copy link
Copy Markdown

johnnyshankman commented Nov 3, 2022

TS keeps complaining about import.meta.env

Why was I linked here from #6194? What is the solve in vite2?

@johnnyshankman
Copy link
Copy Markdown

johnnyshankman commented Nov 3, 2022

Was only able to solve with "types": ["webpack-env", "vite/client"], in my tsconfig? Can we like... put that somewhere? You guys closed all of these issues without explaining the solution at all.

https://vitejs.dev/guide/env-and-mode.html#intellisense-for-typescript

This could not be harder to find and does not explain well what the needs are. I dont even know what Intellisense is but I know I'm using TS.

@johnnyshankman
Copy link
Copy Markdown

https://stackoverflow.com/questions/66039933/typescript-types-for-import-meta-env

@ZeekDaGeek
Copy link
Copy Markdown

Here's what's happening:

By default when you use npm create vite@latest my-vue-app --template *-ts it creates a file called src/vite-env.d.ts. While cleaning up all of the files in src/ it ends up getting deleted.

Here's the original: https://github.com/vitejs/vite/blob/main/packages/create-vite/template-vanilla-ts/src/vite-env.d.ts
While this link is for the vanilla-ts template all of the *-ts templates look similar, check your own template to make sure.

Alternatively you can fix it by adding "vite/client" to your types compilerOptions inside of tsconfig.json. Eg:

{
  "compilerOptions": {
    "types": ["vite/client"],
  }
}

Make sure to leave any additional types intact for other LSP integrations.

For the react-ts template that would look like this:

{
  "compilerOptions": {
    "target": "ESNext",
    "useDefineForClassFields": true,
    "lib": ["DOM", "DOM.Iterable", "ESNext"],
    "allowJs": false,
    "skipLibCheck": true,
    "esModuleInterop": false,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "module": "ESNext",
    "moduleResolution": "Node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react-jsx",
    "types": ["vite/client"],
  },
  "include": ["src"],
  "references": [{ "path": "./tsconfig.node.json" }]
}

If you've never used Vite before it's a very simple mistake to make, a lot of people seem to have the issue, and all of the issues seem to point to here.

Sorry for updating a dead post, closing resolved tickets sucks and results in information going in weird spots when they could instead go into the ticket that shows up on your first Google search.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p1-chore Doesn't change code behavior (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants