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

feat(create-vite): tsconfig support vite.config.ts #6324

Merged
merged 7 commits into from
Jan 17, 2022

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Dec 30, 2021

Description

fix #6311

Additional context

Should we distinguish between build and editor configuration files?


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.

@Niputi Niputi added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 30, 2021
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I think we can cover template-svelte-ts too. We might've forgot to rename the config to vite.config.ts.

@poyoho poyoho requested a review from bluwy December 31, 2021 00:06
bluwy
bluwy previously approved these changes Dec 31, 2021
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM. Might want to fix the typo in the PR title too 👀

@poyoho poyoho changed the title feat(teamplate): tsconfig support vite.config.ts feat(template): tsconfig support vite.config.ts Dec 31, 2021
@poyoho
Copy link
Member Author

poyoho commented Dec 31, 2021

hahahaha

bluwy
bluwy previously approved these changes Jan 1, 2022
ydcjeff
ydcjeff previously approved these changes Jan 6, 2022
Copy link
Contributor

@ydcjeff ydcjeff left a comment

Choose a reason for hiding this comment

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

I think PR title should be feat(create-vite): ...

@poyoho poyoho changed the title feat(template): tsconfig support vite.config.ts feat(create-vite): tsconfig support vite.config.ts Jan 7, 2022
@NMinhNguyen
Copy link
Contributor

Hm, I just tried this in my project and if you import something in vite.config.ts that happens to include @types/node, then you can inadvertently make Node globals available in the browser code like global, etc. so this might not be a good idea after all?

@poyoho
Copy link
Member Author

poyoho commented Jan 7, 2022

you mean is you need to install "@types/node" in the project, and the "@types/node" will make the browser had error type?

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Jan 7, 2022

you mean is you need to install "@types/node" in the project, and the "@types/node" will make the browser had error type?

Sorry, I wasn't very clear. Basically if you import another package and that package has a dependency on @types/node. If you add @types/node to your app yourself, then I don't think you can easily avoid those types from being available (unless you use types tsconfig option or something).

In the former case, if you don't include vite.config.ts in your tsconfig, then type-checking your Vite app shouldn't introduce Node globals (or at least it doesn't if you use Yarn PnP).

@poyoho
Copy link
Member Author

poyoho commented Jan 7, 2022

thanks @NMinhNguyen.

yes, your editor typescript project will no load the vite.config.ts and no aload the @types/node in the typescript project.

I think only add the vite/client reference in the vite.config.ts better 😀

@NMinhNguyen
Copy link
Contributor

Yeah but the thing is that vite.config.ts is run in a Node.js context, so it's likely that you'd need to reference Node-specific things 😅

@poyoho
Copy link
Member Author

poyoho commented Jan 7, 2022

just add vite/client and @types/node but @types/node is not default deps

@NMinhNguyen
Copy link
Contributor

just add vite/client and @types/node but @types/node is not default deps

That would introduce Node globals in the browser code, right?

@poyoho
Copy link
Member Author

poyoho commented Jan 7, 2022

no, because tsconfig.json will no include vite.config.ts, just add reference in the vite.config.ts

@NMinhNguyen
Copy link
Contributor

no, because tsconfig.json will no include vite.config.ts, just add reference in the vite.config.ts

How? Do you mean /// <reference types="node" />? That doesn't work if you don't have @types/node installed, and installing them makes them available in the browser code too.

@poyoho
Copy link
Member Author

poyoho commented Jan 8, 2022

but it can't make the vite.config.ts support module=esnext 😢

@poyoho
Copy link
Member Author

poyoho commented Jan 8, 2022

I find the tsconfig.json had lib config, will ignore @types/node by default?

@NMinhNguyen
Copy link
Contributor

I haven't tried this but types might help. At the same time, this issue isn't like a blocker so maybe it's okay to see some squiggly lines in the editor after all? 😁

@poyoho
Copy link
Member Author

poyoho commented Jan 8, 2022

use tsconfig option reference to split browser and node env, that will no load @types/node in browser env.

@NMinhNguyen
Copy link
Contributor

use tsconfig option reference to split browser and node env, that will no load @types/node in browser env.

Interesting! I'm on my phone atm so can't check but will check later tonight

@NMinhNguyen
Copy link
Contributor

@poyoho I've verified and it seems to work as intended! Great work!

@poyoho poyoho requested a review from bluwy January 10, 2022 23:51
bluwy
bluwy previously approved these changes Jan 11, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

This seems to complicate the setup a bit, but LGTM if it's something the end-user would eventually need to solve.

@patak-dev
Copy link
Member

@poyoho we talked about this PR too :)

This is a good approach, looks like the vue cli starter will also use the same scheme moving forward. Before merging, it would be good to try to simplify the .node.json. Would it be possible to review and make them as minimal as possible? Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using import.meta.url in vite.config.ts shows error in IDE (VS Code)
6 participants