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

Update plugin for Docusaurus 2.0.0-beta.9+ (Webpack 5+, Node 16+, NPM 8+) #51

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

avaidyam
Copy link
Contributor

@avaidyam avaidyam commented Nov 3, 2021

The biggest issues with upgrading the plugin to the Docusaurus v2 betas comes from Webpack 5 and NPM v8 (as part of NodeJS v16+). What's upsetting is the fact that there's near ZERO useful diagnostic information whatsoever.

NOTE: You may need to replace npm install with npm install --legacy-peer-deps on npm 7+ CI/CD workflows before npm publishing.

Notes

  1. Webpack 5 no longer supports automatic Node process/Buffer polyfills.
    • In the OpenAPI plugin, react-markdown depends on vfile, which depends on path, which uses process.platform without an import.
    • There's a lot more dependency rabbit hole issues that can't be solved by upgrading/replacing dependencies, so instead, we should just polypill the two things we need as part of our plugin (buffer and process).
  2. As of NPM v7+ (included with Node v14+), there's a new --legacy-peer-deps flag that needs to be applied to npm install before using npm link for testing the library.
    • When in doubt, triple-check: npm ls @docusaurus/core (^2.0.0-beta.9), npm ls react (^17.0.2), npm ls react-dom (^17.0.2, MUST MATCH react VERSION), and npm ls webpack (^5.61.0).
  3. When static-building (docusaurus build), a cryptic minified React error 321 supposedly indicates to the developer that hooks are not being implemented/used properly. DO NOT BELIEVE THIS ERROR - IT IS A RED HERRING!
  4. Finally, while the OpenAPI page may load, it spews a massive number of warnings to the Docusaurus console:
    <w> [webpack.cache.PackFileCacheStrategy] Skipped not serializable cache item 'Compilation/modules|./docusaurus-plugin-openapi/docusaurus-plugin-openapi/node_modules/**/*.js': No serializer registered for ProvidedDependency
    <w> while serializing webpack/lib/cache/PackFileCacheStrategy.PackContentItems -> webpack/lib/NormalModule -> Array { 16 items } -> ProvidedDependency
    
    • This is not actually a Webpack configuration issue or anything to do with the plugin itself. The ProvidedDependency class of ONE copy of Webpack is not serializable by a SEPARATE copy of Webpack (same version).
    • This essentially means there are two or more Webpack installations in node_modules.

Changelog

  • Replace yarn.lock with package.lock.
  • Update to the latest Docusaurus and React versions.
  • Modernize the Docusaurus config and Markdown sample files (some parameters, like id were deprecated).
  • Remove deprecated routesLoaded lifecycle function.
  • Address useScrollPosition et al. being moved to @docusaurus/theme-common.
  • Address react-markdown being significatly outdated by upgrading to v6 and adding rehype plugins as recommended.
  • Address significant Webpack 5 issues caused by removal of polyfills.
  • Resolve mismatched React and React-DOM version issues (which breaks SSR builds).
  • Resolve duplicate React/ReactDOM/Webpack issues (which generally breaks everything with almost ZERO useful diagnostic information).

- Replace `yarn.lock` with `package.lock`.
- Update to the latest Docusaurus and React versions.
- Modernize the Docusaurus config and Markdown sample files (some parameters, like `id` were deprecated).
- Remove deprecated `routesLoaded` lifecycle function.
- Address `useScrollPosition` et al. being moved to `@docusaurus/theme-common`.
- Address `react-markdown` being significatly outdated by upgrading to v6 and adding `rehype` plugins as recommended.
- Address significant Webpack 5 issues caused by removal of polyfills.
- Resolve mismatched React and React-DOM version issues (which breaks SSR builds).
- Resolve duplicate React/ReactDOM/Webpack issues (which generally breaks everything with almost ZERO useful diagnostic information).
This was referenced Nov 3, 2021
@bourdakos1
Copy link
Member

This looks great! However, I haven't tested locally yet

My main question is why the switch from yarn to npm?

In the OpenAPI plugin, react-markdown depends on vfile, which depends on path, which uses process.platform without an import.

After bumping the markdown package to 7.1.0, I no longer had the webpack process/path issues, but I think there was buffer issues from the DemoPanel

@avaidyam
Copy link
Contributor Author

avaidyam commented Nov 3, 2021

My main question is why the switch from yarn to npm?

Primarily since it's default with NodeJS installations so I imagine it has a wider audience. However, the real reason there's no yarn.lock files in this PR is because I do not have Yarn installed and do not want users to suffer dependency hell from an outdated lock file if they use Yarn.

After bumping the markdown package to 7.1.0, I no longer had the webpack process/path issues, but I think there was buffer issues from the DemoPanel

Yep, this is about right, I went down the list patching things out to determine it was process, path, and buffer. (There's no more path dependency with the latest react-markdown though.)

@bourdakos1
Copy link
Member

Primarily since it's default with NodeJS installations so I imagine it has a wider audience. However, the real reason there's no yarn.lock files in this PR is because I do not have Yarn installed and do not want users to suffer dependency hell from an outdated lock file if they use Yarn.

In that case, can we revert the npm/yarn change? I can push commits to the PR to update the yarn.lock if you don't feel comfortable installing yarn. We can have a separate discussion about npm vs yarn going forward and open a new PR to make that change depending on what we decide.

Otherwise, everything else looks good to me :)

@avaidyam
Copy link
Contributor Author

avaidyam commented Nov 3, 2021

can we revert the npm/yarn change?

Sure.

I can push commits to the PR to update the yarn.lock if you don't feel comfortable installing yarn.

Please do!

We can have a separate discussion about npm vs yarn going forward

To be clear, I'm not entirely opposed to Yarn. I just suggest we support package-lock.json at an absolute minimum, and yarn.lock optionally on top of that. It looks like this can be automated too.

Copy link
Member

@bourdakos1 bourdakos1 left a comment

Choose a reason for hiding this comment

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

Tested locally and looks good. There's a bit of an issue with the sidebar mobile support, but I think we can fix that in a separate PR

@bourdakos1
Copy link
Member

fixes: #31

@bourdakos1 bourdakos1 merged commit df7f8f9 into cloud-annotations:master Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants