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(nuxt): add @types/node as a peerDependency #20025

Merged
merged 2 commits into from Apr 6, 2023
Merged

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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 aims to improve DX by installing @types/node by default (but respecting user's preference if they install a different version, to match their own node version).

πŸ“ Checklist

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

@codesandbox
Copy link

codesandbox bot commented Apr 1, 2023

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@nuxt-studio-dev
Copy link

nuxt-studio-dev bot commented Apr 1, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
Nuxt Docs Edit on Studio β†—οΈŽ View Live Preview 4395922

@nuxt-studio
Copy link

nuxt-studio bot commented Apr 1, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
Nuxt Docs Edit on Studio β†—οΈŽ View Live Preview a43c4eb

@danielroe danielroe requested a review from antfu April 3, 2023 11:20
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Makes sense to me πŸ‘

@pi0
Copy link
Member

pi0 commented Apr 3, 2023

Can't we do this simply by adding typescript and @types/node to default starter templates as devDependendencies? (I often found typescript is missing and IDE not detecting features properly until installing it)

Peer dependency behavior of package managers is sometimes annoying we never added them to nuxt package for this reason.

@danielroe
Copy link
Member Author

danielroe commented Apr 3, 2023

Could we do both (add as peerDep and also in starter)? @types/node is really required for the types to work (e.g. process.dev), but we ideally want to defer to the user to pick which version (16/18/etc.). Even if package manager force installs @types/node, I don't see an issue with it.

@pi0
Copy link
Member

pi0 commented Apr 3, 2023

Isn’t it enough to add dev dependencies to starter templates to fix the issues?

@danielroe
Copy link
Member Author

I think it would fix the issue for new projects, but not affect existing ones.

What are you worried that this change might do?

@pi0
Copy link
Member

pi0 commented Apr 3, 2023

We could simply instruct old projects to install both typescript and @types/node packages as currently documented if this change is purely for them.

I am even up to supporting nuxi typescript setup command to automate local ts setup :)

Package managers never had a persistent and predictable behavior with peer dependency handling. We might need to always keep updating a new range for supported node and for (even small!) fragment of users are not using typescript, it is just an additional dependency forced to install (or see warn)

@danielroe
Copy link
Member Author

danielroe commented Apr 3, 2023

You're right, we would need to keep updating the range. But we currently need to do the exact same thing for engines.node, which is a couple of lines further down the file. I think keeping them in sync isn't too much.

Of course we can tell people to add the dependency themselves (and we should). But I think this is exactly the purpose for which we have peerDeps, right?

I'm still not sure what disadvantages there would be to this. I'm not suggesting adding typescript which would definitely be too much for those not using type-checking. But @types/node is used and picked up by IDEs even if users don't use type-checking, and will result in a broken experience if missing. See, for example #20040 (comment).

I am very happy to revert this change (or enable peerDependenciesMeta.optional, as vite does) if we experience any negative issues.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

TBH I'm still not 100% convinced this is the best way to solve the issue with peerDependencies while simply should be solved in starter (and anyway we need to add devDependency there too) but let's try doing it...

@danielroe danielroe changed the title fix(nuxt): add @types/node as a peerDep fix(nuxt): add @types/node as a peerDependency Apr 6, 2023
@danielroe danielroe merged commit 4d75540 into main Apr 6, 2023
11 checks passed
@danielroe danielroe deleted the chore/node-types branch April 6, 2023 12:33
@danielroe danielroe mentioned this pull request Apr 10, 2023
@kpetrillo
Copy link

I'm still receiving this error after upgrading to v3.4.0. I've also installed @types/node manually as well.

@danielroe
Copy link
Member Author

@kpetrillo I'm not sure what issue you're referring to. Would you create a new issue - with a reproduction?

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

Successfully merging this pull request may close these issues.

None yet

4 participants