feat(gatsby): add pnp resolving by passing rootdir in pkg resolving

[arcanis]

Description

The current resolution algorithm takes loads the plugins using require.resolve via gatsby, meaning that PnP will block them unless they're listed in gatsby's dependencies. This diff ensures that the plugins are loaded from the package that contains the configuration.

Related Issues

#10245

[arcanis]

The fail doesn't seem related, right?

[wardpeet]

Could you merge in master? I don't think you have the latest one.

[arcanis]

All good now 👍

[arcanis]

Anything else needed?

[pieh]

Small thing seems to be missing - in https://github.com/gatsbyjs/gatsby/blob/a67864b087b0966a14ea94df2833b3e6f7bc2b7f/packages/gatsby/src/bootstrap/load-plugins/index.js#L40-L42

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.

[pieh]

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 master if theme plugin packages are not hoisted, but only way those wouldn't be hoisted is (I think - please correct me if I'm wrong) if site and theme (or one theme and another theme) depend on incompatible plugin versions (which would probably cause a lot more issues than jest resolving part).

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.

[pieh]

So let's merge this one in as this doesn't introduce regressions and solves plugin resolution for most common usecase (right now).

[arcanis]

but only way those wouldn't be hoisted is (I think - please correct me if I'm wrong) if site and theme (or one theme and another theme) depend on incompatible plugin versions (which would probably cause a lot more issues than jest resolving part).

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).

[pieh]

Fwiw the exact hoisting algorithm isn't guaranteed by any package manager, so this assumption is unsafe.

That's true. The main point was that we currently rely on plugin packages being installed in main site node_modules right now, and this PR doesn't change that.

There is one case I missed earlier:
We add one of plugins by default in

gatsby/packages/gatsby/src/bootstrap/load-plugins/load.js

Lines 192 to 198 in 32a3705

	processPlugin({
	resolve: `gatsby-plugin-page-creator`,
	options: {
	path: slash(path.join(program.directory, `src/pages`)),
	pathCheck: false,
	},
	})

and this one is in gatsby dependencies -

gatsby/packages/gatsby/package.json

Line 69 in 32a3705

"gatsby-plugin-page-creator": "^2.0.8",

If I understand correctly - we need to resolve that from gatsby package directory and not from site directory? I.e. change:

  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,
      },
    })
  )

[arcanis]

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]

@arcanis Does #12315 remove the need for this?

[arcanis]

@sidharthachatterjee no, both PRs are orthogonal (I'll leave more details on the other one)

[pieh]

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!

[arcanis]

Awesome, thanks!