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.

Sign up for GitHub

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

Jump to bottom

fix: declare missing dependencies #6097

Merged
merged 5 commits into from
Dec 13, 2021
Merged

fix: declare missing dependencies #6097

merged 5 commits into from
Dec 13, 2021

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented Dec 11, 2021

Motivation

#6047 made Docusaurus PnP strict mode compatible but by default PnP still allows some fallbacks, this PR disables that fallback in the e2e test and adds the remaining undeclared dependencies.

Have you read the [Contributing Guidelines on pull requests]

Yes

Test Plan

The e2e test passes

@netlify
Copy link

netlify bot commented Dec 11, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: bd6e9cb

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61b4b8c1582f2c0007b3fdf9

😎 Browse the preview: https://deploy-preview-6097--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Dec 11, 2021 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 93
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-6097--docusaurus-2.netlify.app/

@netlify
Copy link

netlify bot commented Dec 11, 2021 •
edited
Loading

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: fb24179

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61b55466fe13cb0007f822cf

😎 Browse the preview: https://deploy-preview-6097--docusaurus-2.netlify.app

@merceyz merceyz mentioned this pull request Dec 11, 2021
Merged
3 tasks
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 11, 2021
@Josh-Cena
Copy link
Collaborator

Thanks, do you think it makes more sense to declare react as optional peer deps in @docusaurus/utils, since there's only one function that actually relies on it and it's hardly used?

@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Dec 12, 2021
@merceyz
Copy link
Contributor Author

merceyz commented Dec 12, 2021 •
edited
Loading

It's currently always imported so no, if it was changed to use dynamic imports then sure.

Josh-Cena
Josh-Cena approved these changes Dec 13, 2021
View reviewed changes
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.

I see

@Josh-Cena Josh-Cena merged commit b48c6de into facebook:main Dec 13, 2021
@merceyz merceyz deleted the merceyz/fix/pnp-strict branch December 13, 2021 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Josh-Cena Josh-Cena Josh-Cena approved these changes

@lex111 lex111 Awaiting requested review from lex111

@slorber slorber Awaiting requested review from slorber slorber is a code owner

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@merceyz @Josh-Cena @facebook-github-bot