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(schema): add more explicit types for config schema #21475

Merged
merged 7 commits into from Jun 8, 2023
Merged

fix(schema): add more explicit types for config schema #21475

merged 7 commits into from Jun 8, 2023

Conversation

Lehoczky
Copy link
Contributor

@Lehoczky Lehoczky commented Jun 8, 2023

πŸ”— Linked issue

Follow-up for #21363

❓ 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 adds or updates the type annotation for the config properties wherever it was achievable (some webpack loaders for example don't have their own type annotations).

For other details, see my comments below.

πŸ“ Checklist

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

@@ -48,7 +48,7 @@ export function viteNodePlugin (ctx: ViteBuildContext): VitePlugin {
markInvalidates(server.moduleGraph.getModulesByFile(typeof plugin === 'string' ? plugin : plugin.src))
}
for (const template of ctx.nuxt.options.build.templates) {
markInvalidates(server.moduleGraph.getModulesByFile(template?.src))
markInvalidates(server.moduleGraph.getModulesByFile(template?.src as string))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

because ctx.nuxt.options.build.templates is no longer any, TypeScript shows an error here, because the src in NuxtTemplate can be undefined.

I'm guilty of not trying out what happens (or even can it happen) if it is undefined here, but I can investigate it further.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. We do need to investigate - is it possible that the module would be added by template.dst instead? πŸ€” This may be the origin of a bug we're encountering elsewhere...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By quickly looking at the code, I think we expect to use template.getContents when src is not present (see), and we call that function at one place in the codebase, but we never set src to anything if it is undefined, so we can potentially call server.moduleGraph.getModulesByFile() with undefined.

Although yeah, I just glanced over the code, I'm gonna check this out more thoroughly tomorrow.

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

This is a great PR - many thanks for it ❀️

@Lehoczky
Copy link
Contributor Author

Lehoczky commented Jun 8, 2023

This is a great PR - many thanks for it ❀️

My pleasure @danielroe <3

@danielroe danielroe changed the title fix(schema): update defineNuxtConfig() types fix(schema): add more explicit types for config schema Jun 8, 2023
@danielroe danielroe merged commit 7ff0c2d into nuxt:main Jun 8, 2023
22 checks passed
@github-actions github-actions bot mentioned this pull request Jun 8, 2023
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.

None yet

2 participants