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(create-docusaurus): add missing dependencies to templates #7539

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented May 31, 2022

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 to
    import 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 from react
    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)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 31, 2022
@netlify
Copy link

netlify bot commented May 31, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit d10a3d9
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62ac64200d214500095e16f5
😎 Deploy Preview https://deploy-preview-7539--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 31, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 71 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 80 🟢 99 🟢 100 🟢 100 🟢 90 Report

Copy link
Collaborator

@Josh-Cena Josh-Cena left a 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!)

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Jun 1, 2022
@@ -3719,6 +3724,15 @@
"@types/scheduler" "*"
csstype "^3.0.2"

"@types/react@^17":
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@merceyz
Copy link
Contributor Author

merceyz commented Jun 2, 2022

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

It can be "hidden" by having @docusaurus/preset-classic expose it and a .d.ts file in the template that references it instead of the types config in tsconfig.json (see how Next.js does this) but as it stands right now it's required.

(And we have a lot of "below average" users that don't use PnP and don't understand how it works!)

While that's fair this is an issue with node_modules as well.

@Josh-Cena
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yarn PNP and TypeScript doesn't compile out of the box
3 participants