-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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(create-docusaurus): add missing dependencies to templates #7539
base: main
Are you sure you want to change the base?
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'm glad we can fix this issue.
However, I wonder if it's necessary to actually add @docusaurus/theme-classic
to the dependencies? To an average user it may be confusing why this is needed, because this dependency is kind of hidden in the shareable tsconfig. (And we have a lot of "below average" users that don't use PnP and don't understand how it works!)
@@ -3719,6 +3724,15 @@ | |||
"@types/scheduler" "*" | |||
csstype "^3.0.2" | |||
|
|||
"@types/react@^17": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't necessarily want to let users use @types/react@17
. Does it work if they install @types/react@18
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely but the template depends on react@^17.0.2
so the types should match that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, it's also speaking for ourselves. We do use @types/react@18
in our code base (for other strictness it brings) so having duplicates doesn't seem ideal for us. But I see the reason for most others.
It can be "hidden" by having
While that's fair this is an issue with |
I see... I'll think about in the coming days about what we shall do. I think we should re-export all types from preset-classic. |
Pre-flight checklist
Motivation
Resolve the type checking errors in the templates by adding undeclared dependencies.
Motivations for the changes made:
JS and TS templates must depend on
@docusaurus/types
since they try toimport from it in
docusaurus.config.js
.The TS template must depend on
@docusaurus/theme-classic
and@types/node
since
@tsconfig/docusaurus
specifies that they need to be loaded.The TS template must depend on
@types/react
since it imports fromreact
and also needs to provide it to
@docusaurus/theme-classic
for its types.The
@types/react
resolutions
entry is needed because other@types
packages depend on
@types/react@*
which causes duplicated/incompatible types.Test Plan
e2e test should pass.
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
#7521
#6157 (comment)