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(v2): use require.resolve for all webpack presets and plugins #2788

Merged
merged 1 commit into from
May 22, 2020
Merged

fix(v2): use require.resolve for all webpack presets and plugins #2788

merged 1 commit into from
May 22, 2020

Conversation

SamChou19815
Copy link
Contributor

@SamChou19815 SamChou19815 commented May 22, 2020

Motivation

Issue #2576 shows that docusaurus v2 doesn't work with Yarn v2 with pnp enabled.

This diff doesn't try to resolve the issue of the plugin system caused by the incompatibility: it can be solved by using the pnpLoose mode for now. Instead, it will fix some of the webpack plugin loading issue related to yarn pnp.

Inside the webpack config file, it refers to presets and plugins by strings, like '@babel/plugin-transform-runtime'. This setup happens to work with the traditional package managers' behavior on node_modules, which tries to hoist @babel/plugin-transform-runtime to the top-level node_modules if it can. However, it won't work under pnp, which expects that all dependencies are explictly declared.

In this case, since it's declared in a string form, it's webpack instead of @docusaurus/core that will resolve this module to an absolute path. Since webpack doesn't declare @babel/plugin-transform-runtime as dependency, the resolution failed. By using require.resolve, we ensure that @docusaurus/core will resolve the path. Since @docusaurus/core declared @babel/plugin-transform-runtime as a dependency, the resolution will succeed and results in a correct absolute path for webpack to consume.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

I have setup a repo to show that these are the minimal changes necessary to make docusaurus work under pnpMode: loose.

Also, all the tests still passes and deploy preview still works.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 22, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 22, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 4943ae2

https://deploy-preview-2788--docusaurus-2.netlify.app

@SamChou19815 SamChou19815 marked this pull request as ready for review May 22, 2020 00:40
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Nice one thanks Sam!

@yangshun yangshun merged commit a7925f2 into facebook:master May 22, 2020
@SamChou19815 SamChou19815 deleted the use-require-resolve-for-webpack-presets-and-plugins branch May 22, 2020 03:50
@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label May 22, 2020
@pranaygp
Copy link

Can we cut a new @next release with this commit @SamChou19815 @yangshun?

I'm using pnpMode: loose but I'm blocked on this

@SamChou19815
Copy link
Contributor Author

Can we cut a new @next release with this commit @SamChou19815 @yangshun?

I'm using pnpMode: loose but I'm blocked on this

@pranaygp I tested on my local machine with various docusaurus packages unplugged and with my changes manually applied, and it can run in pnp loose mode. However I do not control the release process.

@yangshun
Copy link
Contributor

We'll cut a new release within the next few days.

@pranaygp
Copy link

pranaygp commented May 27, 2020

@SamChou19815 I followed the instructions on your repo and it worked like a charm. It's rather annoying to have to use pnpify (I haven't needed it for anything else so far) but I'm glad it works for now

@lex111 lex111 added this to the v2.0.0-alpha.56 milestone May 27, 2020
@yangshun
Copy link
Contributor

yangshun commented May 28, 2020

@SamChou19815 would the previous plugin import syntax still work at all? If not we will have to update the docs asap to prevent more reports like #2825.

I'm wondering whether we should change the default template's docusaurus.config.js as well. Please advise!

@SamChou19815
Copy link
Contributor Author

SamChou19815 commented May 28, 2020

@SamChou19815 would the previous plugin import syntax still work at all? If not we will have to update the docs asap to prevent more reports like #2825.

I'm wondering whether we should change the default template's docusaurus.config.js as well. Please advise!

docusaurus.config.js doesn't need to use require.resolve since I already use createRequire in #2789.

In that specific issue, it might be the case that they are using a version that's too old. There might be some misconfiguration since both yarn and pnp doesn't install the latest version of those plugins.

Managed to reproduce. Working on a fix. createRequire appears to be used in a slightly wrong way, and weirdly this problem is not caught in this repo probably due to the yarn workspace setup.

@yangshun
Copy link
Contributor

docusaurus.config.js doesn't need to use require.resolve since I already use createRequire in #2789.

Ahh I forgot about that. Thanks for the reminder.

Managed to reproduce. Working on a fix. createRequire appears to be used in a slightly wrong way, and weirdly this problem is not caught in this repo probably due to the yarn workspace setup.

Got it, thank you!

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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants