-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(gatsby): add pnp resolving by passing rootdir in pkg resolving #12163
Conversation
The fail doesn't seem related, right? |
Could you merge in master? I don't think you have the latest one. |
All good now 👍 |
Anything else needed? |
Small thing seems to be missing - in gatsby/packages/gatsby/src/bootstrap/load-plugins/index.js Lines 40 to 42 in a67864b
we should have: module.exports = async (config = {}, rootDir) => {
// Collate internal plugins, site config plugins, site default plugins
const plugins = loadPlugins(config, rootDir) I also need to check how themes will behave -they will have their own gatsby-config with plugins and package.json, so plugins defined in themes would need to be resolved relative to theme directory and not root directory I think. Just not sure if we track that when we are merging multiple gatsby-config files. |
Ok, so as suspected, we currently break in I still think we will need to add tracking where plugin is being declared - because this alone will work with standalone Gatsby sites, but will break when themes are being used and PnP is being used. |
So let's merge this one in as this doesn't introduce regressions and solves plugin resolution for most common usecase (right now). |
Fwiw the exact hoisting algorithm isn't guaranteed by any package manager, so this assumption is unsafe. While it usually works kinda like you would expect, the case you mention (conflict between one theme and another) might happen. In fact, that's part of what ESLint is working to fix in their next release (eslint/eslint#11388). |
That's true. The main point was that we currently rely on plugin packages being installed in main site There is one case I missed earlier: gatsby/packages/gatsby/src/bootstrap/load-plugins/load.js Lines 192 to 198 in 32a3705
and this one is in gatsby/packages/gatsby/package.json Line 69 in 32a3705
If I understand correctly - we need to resolve that from plugins.push(
processPlugin({
- resolve: `gatsby-plugin-page-creator`,
+ resolve: require.resolve(`gatsby-plugin-page-creator`),
options: {
path: slash(path.join(program.directory, `src/pages`)),
pathCheck: false,
},
})
) |
Good catch! I didn't spot it as I've added a Gatsby fallback in my PnP code - but the less reasons there is for such a mechanism the better it is 😃 |
@sidharthachatterjee no, both PRs are orthogonal (I'll leave more details on the other one) |
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.
Thanks! We will probably need some adjustments for themes later on, but this doesn't break things when not using PnP, so let's ship this!
Awesome, thanks! |
## 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](https://github.com/arcanis/pnp-webpack-plugin/blob/master/index.js#L110-L138) when not operating within a PnP environment, so it shouldn't break any existing workflow 🙂 ## Related Issues Part of #10245 Depends on #12163
Description
The current resolution algorithm takes loads the plugins using
require.resolve
viagatsby
, meaning that PnP will block them unless they're listed ingatsby
's dependencies. This diff ensures that the plugins are loaded from the package that contains the configuration.Related Issues
#10245