fix: make Docusaurus PnP strict mode compatible #6047

Josh-Cena · 2021-12-04T09:02:33Z

Motivation

Close #2576. This PR did the following changes

Moved the getFileLoaderUtils function from core/webpack/utils to @docusaurus/utils so that the mdx loader doesn't depend on the core which will cause circular dependencies otherwise.
Made all Webpack loaders imported with require.resolve so that all the extra loader dependencies can be safely removed from the init templates.
Moved core/constants to @docusaurus/utils so that constants are not duplicated.
Made @docusaurus/core the peer dependency instead of hard dependency of any plugin.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tested locally & updated the E2E test script

netlify · 2021-12-04T09:43:26Z

✔️ [V2]

🔨 Explore the source changes: a700cf1

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

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

github-actions · 2021-12-04T09:43:47Z

⚡️ Lighthouse report for the changes in this PR:

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

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

packages/docusaurus-utils/src/webpackUtils.ts

Co-authored-by: Kristoffer K. <[email protected]>

packages/docusaurus-utils/src/webpackUtils.ts

packages/docusaurus-plugin-client-redirects/package.json

slorber

Thanks, that's really great to get rid of these cycles!

Just asked a few questions to be sure

.github/workflows/tests-e2e.yml

slorber · 2021-12-08T11:47:29Z

...s/docusaurus-mdx-loader/src/remark/transformImage/__tests__/__snapshots__/index.test.ts.snap

@@ -12,17 +12,17 @@ exports[`transformImage plugin pathname protocol 1`] = `
 exports[`transformImage plugin transform md images to <img /> 1`] = `
 "![img](https://example.com/img.png)

-<img src={require(\\"!url-loader?limit=10000&name=assets/images/[name]-[hash].[ext]&fallback=file-loader!./static/img.png\\").default} />
+<img src={require(\\"![CWD]/node_modules/url-loader/dist/cjs.js?limit=10000&name=assets/images/[name]-[hash].[ext]&fallback=[CWD]/node_modules/file-loader/dist/cjs.js!./static/img.png\\").default} />


wonder if this snapshot is normal? What is this new syntax?

It's just require.resolve('url-loader') + replacing CWD with placeholder so the absolute path isn't hardcoded in the snapshot

slorber · 2021-12-08T11:48:37Z

packages/docusaurus-mdx-loader/src/remark/transformImage/__tests__/index.test.ts

-  return result.toString();
+  return result
+    .toString()
+    .replace(new RegExp(posixPath(process.cwd()), 'g'), '[CWD]');


I don't understand that part.
Is the goal to make tests insensitive to local FS?

Yes, indeed

packages/docusaurus-plugin-client-redirects/package.json

slorber · 2021-12-08T11:52:39Z

packages/docusaurus-plugin-client-redirects/src/normalizePluginOptions.ts

@@ -7,7 +7,7 @@

 import {PluginOptions, RedirectOption, UserPluginOptions} from './types';
 import {Joi, PathnameSchema} from '@docusaurus/utils-validation';
-import {DEFAULT_PLUGIN_ID} from '@docusaurus/core/lib/constants';
+import {DEFAULT_PLUGIN_ID} from '@docusaurus/utils';


I was wondering if we shouldn't have a dedicated @docusaurus/constants package? any thought 🤷‍♂️ ?

Hmmm, I don't like little packages everywhere... @docusaurus/utils serves a very clear purpose as a general util to be consumed by any Node code, so I think it makes sense to put server-side constants here

slorber · 2021-12-08T11:55:11Z

packages/docusaurus-theme-live-codeblock/package.json

@@ -29,10 +28,12 @@
    "react-live": "2.2.3"
  },
  "devDependencies": {
+    "@docusaurus/core": "2.0.0-beta.9",


how do you choose if it's needed in devDependencies? If there are tests?

This is a hack so that lerna correctly figures out the dependency graph and builds the core before building those that still implicitly depend on the core's internal APIs (that's still bad, we need to further refactor that)

packages/docusaurus-theme-live-codeblock/src/theme/Playground/index.js

slorber · 2021-12-08T13:26:15Z

LGTM 👍

Josh-Cena · 2021-12-09T13:32:14Z

Okay, this is slightly problematic 😅 @slorber lerna doesn't seem to bump peerDeps during publish: lerna/lerna#955 this doesn't cause installation errors but still triggers some warnings. Maybe we want to use caret ranges for peerDeps?

slorber · 2021-12-09T14:36:42Z

I'm not sure to know what to do here

What kind of warning will users have? Will all package managers work?

I've seen some tools using '*' for peerDeps, and we could use a semver range for sure, but maybe fixed versions are better to ensure correct version resolution in all cases, instead of leading to an error when trying to run Docusaurus?

React seems to use a fixed version like us for react/react-dom: https://github.com/facebook/react/blob/main/packages/react-dom/package.json

🤷‍♂️

What is the advantages of using peerDeps in this case compared to what we had before? Does reverting this change prevent the usage of PnP strict mode?

(You see, this is exactly why changes like upgrading to Yarn 3 can unexpectedly become a time sink 😄)

Josh-Cena · 2021-12-09T14:47:12Z

What kind of warning will users have? Will all package managers work?

➤ YN0060: │ website@workspace:. provides @docusaurus/core (pade89) with version 2.0.0-beta.10, which doesn't satisfy what @docusaurus/preset-classic and some of its descendants request

This is for a project on Yarn v3.

warning " > @docusaurus/[email protected]" has incorrect peer dependency "@docusaurus/[email protected]".

This is in our own repo. (The only Docusaurus project on Yarn v1 that I have.)

It doesn't prevent PnP; it just makes package managers frown. I haven't seen any of them error. But I know that pnpm is stricter about peer deps and they may be unhappy.

I've seen some tools using '*' for peerDeps, and we could use a semver range for sure, but maybe fixed versions are better to ensure correct version resolution in all cases, instead of leading to an error when trying to run Docusaurus?

Yes, a fixed peer dependency does make sense; the only problem being that lerna refuses to bump peer dependencies, which I can also sympathize with. Still, we have runtime checks so it's not strictly necessary.

What is the advantages of using peerDeps in this case compared to what we had before? Does reverting this change prevent the usage of PnP strict mode?

No; I'm fine with reverting back to hard dep. As I said, it's just semantically better because plugins should work in isolation from the core.

(You see, this is exactly why changes like upgrading to Yarn 3 can unexpectedly become a time sink 😄)

It's not about Yarn v3, but exactly about lerna which we hope to ditch in the Yarn v3 migration. Basically, what package manager we use should minimally affect the end users.

Josh-Cena added 2 commits December 4, 2021 16:38

fix: make Docusaurus pnp strict mode compatible

4af0b6b

Add deps on tslib

6bbcaa7

Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Dec 4, 2021

Josh-Cena requested review from lex111 and slorber as code owners December 4, 2021 09:02

facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 4, 2021

Josh-Cena added 4 commits December 4, 2021 17:06

Unbreak?

a2f19bb

This has to happen...

507eda4

Reorg deps

a700cf1

Fixes

3bca988

This comment has been minimized.

Sign in to view

facebook deleted a comment from netlify bot Dec 4, 2021

Josh-Cena added 9 commits December 4, 2021 17:54

Fix

49873a0

Fix deps

5b88162

Fix test

868dbb6

Fix

aaa3e9d

Upgrade Babel

50be91e

Fix peerDeps

ea3011c

Remove deps

12ababf

Fix again...

f2a4734

Improve comment

0766f32

Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 4, 2021

Josh-Cena changed the title ~~fix: make Docusaurus pnp strict mode compatible~~ fix: make Docusaurus PnP strict mode compatible Dec 4, 2021

Merge branch 'main' into jc/yarn-berry

daec8d9

Josh-Cena mentioned this pull request Dec 6, 2021

chore: migrate to Yarn Modern #6028

Closed

merceyz reviewed Dec 6, 2021

View reviewed changes

packages/docusaurus-utils/src/webpackUtils.ts Outdated Show resolved Hide resolved

Josh-Cena and others added 2 commits December 6, 2021 09:47

Update packages/docusaurus-utils/src/webpackUtils.ts

623678e

Co-authored-by: Kristoffer K. <[email protected]>

Can we remove them?

5611786

Nope

1b1d4e8

Josh-Cena mentioned this pull request Dec 6, 2021

chore: remove svgr/webpack from deps of new project #5703

Closed

merceyz reviewed Dec 6, 2021

View reviewed changes

packages/docusaurus-utils/src/webpackUtils.ts Outdated Show resolved Hide resolved

Josh-Cena added 5 commits December 6, 2021 20:02

Does it work?

a4eebb6

Fix tests

a9c3263

Fix again

8f8dc18

Okay...

9ab927b

Hope this is the last time

ec7d380

Josh-Cena linked an issue Dec 7, 2021 that may be closed by this pull request

[webpack 5] Yarn v2 can't resolve file loader in docs plugin #4778

Closed

Move core to peerDeps & constants to utils

9bd355f

Josh-Cena commented Dec 7, 2021

View reviewed changes

packages/docusaurus-plugin-client-redirects/package.json Show resolved Hide resolved

Josh-Cena added 2 commits December 7, 2021 22:34

Remove other dup constants

0abf912

Add some core devDeps

cba0b9f

slorber approved these changes Dec 8, 2021

View reviewed changes

Update tests-e2e.yml

0a9742c

slorber merged commit e07ebad into main Dec 8, 2021

slorber deleted the jc/yarn-berry branch December 8, 2021 13:26

Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 8, 2021

Josh-Cena mentioned this pull request Dec 9, 2021

chore(v2): prepare v2.0.0-beta.10 release #6076

Merged

This was referenced Dec 9, 2021

Can't NPM Install with 2.0.0-beta.10 #6079

Closed

fix: move docusaurus core back to hard dependencies #6078

Merged

This was referenced Dec 11, 2021

fix: declare missing dependencies #6097

Merged

ci: add Docusaurus e2e test yarnpkg/berry#3863

Merged

Josh-Cena mentioned this pull request Dec 12, 2021

fix: make plugin compatible with Docusaurus beta.13 cloud-annotations/docusaurus-openapi#88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make Docusaurus PnP strict mode compatible #6047

fix: make Docusaurus PnP strict mode compatible #6047

Josh-Cena commented Dec 4, 2021 •

edited

Loading

netlify bot commented Dec 4, 2021

github-actions bot commented Dec 4, 2021 •

edited

Loading

This comment has been minimized.

slorber left a comment

slorber Dec 8, 2021

Josh-Cena Dec 8, 2021

slorber Dec 8, 2021

Josh-Cena Dec 8, 2021

slorber Dec 8, 2021

Josh-Cena Dec 8, 2021

slorber Dec 8, 2021

Josh-Cena Dec 8, 2021

slorber commented Dec 8, 2021

Josh-Cena commented Dec 9, 2021

slorber commented Dec 9, 2021

Josh-Cena commented Dec 9, 2021

fix: make Docusaurus PnP strict mode compatible #6047

fix: make Docusaurus PnP strict mode compatible #6047

Conversation

Josh-Cena commented Dec 4, 2021 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

netlify bot commented Dec 4, 2021

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

This comment has been minimized.

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber commented Dec 8, 2021

Josh-Cena commented Dec 9, 2021

slorber commented Dec 9, 2021

Josh-Cena commented Dec 9, 2021

Josh-Cena commented Dec 4, 2021 •

edited

Loading

github-actions bot commented Dec 4, 2021 •

edited

Loading