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鈥檒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

Merged
merged 5 commits into from
Mar 15, 2019

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Mar 5, 2019

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

@arcanis arcanis requested a review from a team as a code owner March 5, 2019 13:09
@@ -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),
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a 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,
Copy link
Contributor

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?

Copy link
Contributor Author

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}/`)],
Copy link
Contributor

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

Copy link
Contributor Author

@arcanis arcanis Mar 6, 2019

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 馃檪

@arcanis
Copy link
Contributor Author

arcanis commented Mar 6, 2019

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 require call.

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 require, and thus they cannot find the location of the packages being required.

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 resolve.plugins and resolveLoader.plugins keys. Thanks to this, I've been able to write pnp-webpack-plugin which leverage the PnP resolution if it finds out it's running under it (it's pretty easy because PnP publicly exposes a special pnpapi builtin module that references the PnP API under which you're operating).

I'll add more details in the comments on this PR, and put another commit to add inline comments as well 馃憤

@arcanis
Copy link
Contributor Author

arcanis commented Mar 8, 2019

@sidharthachatterjee What do you think? 馃檪

@arcanis
Copy link
Contributor Author

arcanis commented Mar 15, 2019

Ping?

@DSchau
Copy link
Contributor

DSchau commented Mar 15, 2019

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!

@arcanis
Copy link
Contributor Author

arcanis commented Mar 15, 2019

No worry! Updated 馃憤

@DSchau DSchau changed the title Adds zero-config pnp support feat(gatsby): set up webpack config for eventual PnP support Mar 15, 2019
Copy link
Contributor

@DSchau DSchau left a 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!

@DSchau DSchau merged commit ad6319b into gatsbyjs:master Mar 15, 2019
@DSchau
Copy link
Contributor

DSchau commented Mar 15, 2019

Published as gatsby@2.1.35

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.

None yet

3 participants