-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(gatsby): set up webpack config for eventual PnP support #12315
Conversation
55bc907
to
d8c3165
Compare
@@ -332,6 +333,11 @@ module.exports = async ( | |||
), | |||
"create-react-context": directoryPath(`.cache/create-react-context.js`), | |||
}, | |||
plugins: [ | |||
PnpWebpackPlugin.bind(directoryPath(`.cache`), module), | |||
PnpWebpackPlugin.bind(directoryPath(`public`), module), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary for public
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc some files in public
depend on gatsby-link
, which is a gatsby
dependency rather than something listed in the user project's package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't have a lot of context here with respect to yarn pnp
Could you elaborate on what the plugin does and maybe leave a few comments for future readers? 馃檪
Thank you for this, @arcanis
plugins: [ | ||
PnpWebpackPlugin.bind(directoryPath(`.cache`), module), | ||
PnpWebpackPlugin.bind(directoryPath(`public`), module), | ||
PnpWebpackPlugin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this instance do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With PnP one important thing is that a file is only allowed to require the third-party that its package depends on. So for example you can't accidentally require('lodash')
if you forgot to list it in your dependencies (which would work otherwise but could break in production if the lodash you were relying on was obtained via a dev dependency).
In order to do this, we rely on the path of the module that makes the require call. Since Yarn knows the location of all the packages in your dependency tree, we can safely deduce that if a package is installed in /X/Y
, then a file located in /X/Y/Z.js
can only require the files listed in /X/Y/package.json
.
Unfortunately Gatsby breaks this model by creating files within the user folders. Those files are copied from the gatsby
package (gonna be honest I'm still not entirely sure why they are copied in the first place rather than used from their original location), and put inside the user folder. What this mean is that when /X/Y/.cache/default-html.js
makes a require to something, Yarn thinks that the dependency should be resolved relative to /X/Y/package.json
- even though it should instead be from something like /cache/yarn/gatsby-1.2.3/package.json
.
The bind
directive instruct the Webpack PnP plugin that everything in the specified directory path should be resolved as if it was the specified module object. Since src/utils/webpack.config.js
is kept within the Gatsby package folder, the cache files require calls are resolved relative to the dependencies listed in it and everything works.
@@ -350,6 +356,7 @@ module.exports = async ( | |||
|
|||
return { | |||
modules: [...root, path.join(__dirname, `../loaders`), `node_modules`], | |||
plugins: [PnpWebpackPlugin.moduleLoader(`${directory}/`)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this does, add a comment maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem/quirk with how Webpack makes loaders interact with loader plugins is that when it tries to resolve a loader, the "issuer" (the file that makes the require call) is set to the file that makes the require to the imported file (rather than the file that defines the loader - which would arguably be hard to figure out). It means that if a file from a third-party package requires another file that would go through a loader, the call will fail (because the third-party package wouldn't list the loader in its dependencies).
This line simply instructs Webpack that the non-absolute loaders should all be resolved relative to the directory of the application, regardless of who their issue is. It's not a problem for third-party-provided loaders (such as the ones added by Gatsby) because they use require.resolve
anyway, which shortcuts this resolution 馃檪
You're right, I kinda forgot to explain the why here 馃槃 To put back some context: the way PnP works is that we're keeping the third-party files where they are (the actual location is unspecified, but for simplicity we can refer to it as "the Yarn cache"), and generate a JS loader that tells Node where to find those files when it resolves a The problem is that some tools don't play well with that because they reimplement the node resolution themselves - so even if we make it so the Node APIs have the same behavior (which we do), those tools won't benefit from it. Webpack and co are in this case: they simply assume the third-parties to be in the node_modules directory, regardless of the implementation of Fortunately, tools that reimplement the node resolution typically are mature enough that they offer some way to hook into the resolution process and extend it. In the case of Webpack, it comes with the I'll add more details in the comments on this PR, and put another commit to add inline comments as well 馃憤 |
@sidharthachatterjee What do you think? 馃檪 |
Ping? |
Sorry about this! @arcanis would you mind fixing up the merge conflicts? Then we can get this merged in. If you're busy--no worries, I can do it too! |
No worry! Updated 馃憤 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this locally, and does indeed appear to be a no-op in non-PnP environments.
Let's get this merged, since it does pave the road for eventual PnP support--which is something that would be nice to have!
Published as |
Description
This diff adds
pnp-webpack-plugin
in the configuration so that users don't have to manually extend the Webpack configuration to support PnP.Note that the plugin is a strict no-op when not operating within a PnP environment, so it shouldn't break any existing workflow 馃檪
Related Issues
Part of #10245
Depends on #12163