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(vite): ignore and warn if vite config file exists #21588

Merged
merged 2 commits into from Jun 16, 2023

Conversation

NozomuIkuta
Copy link
Contributor

@NozomuIkuta NozomuIkuta commented Jun 15, 2023

πŸ”— Linked issue

Resolves #21553

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR is created based on #21553 (comment).

We can let Vite ignore its config file when calling build() and createServer() by setting configFile: false.

I tested my logic by adding vite.config.js to playground directory and run pnpm -w run play:build to check if the warning message is shown.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Should I add some information to docs?

Comment on lines 29 to 43
// https://github.com/vitejs/vite/blob/8fe69524d25d45290179175ba9b9956cbce87a91/packages/vite/src/node/constants.ts#L38
[
'vite.config.js',
'vite.config.mjs',
'vite.config.ts',
'vite.config.cjs',
'vite.config.mts',
'vite.config.cts'
].some((fileName) => {
if (existsSync(resolve(nuxt.options.rootDir, fileName))) {
consola.warn(`\`${fileName}\` is not supported. Use \`options.vite\` instead. You can read more in \`https://nuxt.com/docs/api/configuration/nuxt-config#vite\`.`)
return true
}
return false
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we could use Vite's loadConfigFromFile() instead, but I was not sure how to align its arguments and Nuxt things. πŸ€”

https://github.com/vitejs/vite/blob/8fe69524d25d45290179175ba9b9956cbce87a91/packages/vite/src/node/config.ts#L896

Would anyone give me a help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we could use Vite's loadConfigFromFile() instead

I mean, it will return null if there is no config file found.

Copy link
Member

Choose a reason for hiding this comment

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

I think your approach is more minimal, and the warning is a 'nice-to-have' extra so it is fine if we don't detect an edge case.

You could consider using resolvePath from mlly or Nuxt Kit to resolve the config file which might simplify the implementation here.

@NozomuIkuta
Copy link
Contributor Author

NozomuIkuta commented Jun 15, 2023

Should I apply this kind of logic to webpack package too?

@Hebilicious
Copy link
Member

Hebilicious commented Jun 15, 2023

It would be nice if we had a Build section in the docs (similar to the deployment section), that goes more into the details of how the build process works.
And within it, there should be a vite section that explains that you can't have a vite.config file and that to modify the vite config, you have some access through nuxt.config.ts, but you need to use the hooks to have full control

@danielroe
Copy link
Member

Should I apply this kind of logic to webpack package too?

Absolutely, if webpack has the same behaviour. Though I think we could do it in a separate PR.

@danielroe danielroe added the 3.x label Jun 15, 2023
@danielroe danielroe requested a review from antfu June 16, 2023 11:17
@danielroe danielroe merged commit 48c2b45 into nuxt:main Jun 16, 2023
22 checks passed
@github-actions github-actions bot mentioned this pull request Jun 16, 2023
@NozomuIkuta NozomuIkuta deleted the chore/add-warning-for-bundler-config branch June 16, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

At least one <template> or <script> is required in a single file component
3 participants