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(gatsby): fix webpack compilation when pnpm is used #38757

Merged
merged 9 commits into from
Jan 10, 2024
44 changes: 44 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,9 @@ jobs:
- run:
command: cp -r ./starters/default /tmp/e2e-tests/gatsby-pnp
working_directory: ~/project
- run: # default doesn't have API functions so let's get some of those
command: cp -r ./e2e-tests/adapters/src/api /tmp/e2e-tests/gatsby-pnp/src/api
working_directory: ~/project
- run:
command: touch yarn.lock
working_directory: /tmp/e2e-tests/gatsby-pnp
Expand Down Expand Up @@ -379,6 +382,45 @@ jobs:
command: 'DEBUG=start-server-and-test yarn start-server-and-test "yarn develop 2>&1 | tee log.txt" :8000 "! cat log.txt | grep -E ''ERROR #|Require stack:''"'
working_directory: /tmp/e2e-tests/gatsby-pnp

e2e_tests_pnpm:
executor:
name: node
image: "18.12.0"
steps:
- checkout
- run: ./scripts/assert-changed-files.sh "packages/*|.circleci/*"
- <<: *attach_to_bootstrap
- run:
command: mkdir -p /tmp/e2e-tests/
working_directory: ~/project
- run:
command: cp -r ./starters/default /tmp/e2e-tests/gatsby-pnpm
working_directory: ~/project
- run: # default doesn't have API functions so let's get some of those
command: cp -r ./e2e-tests/adapters/src/api /tmp/e2e-tests/gatsby-pnpm/src/api
working_directory: ~/project
- run:
command: rm package-lock.json
working_directory: /tmp/e2e-tests/gatsby-pnpm
- run: # Install pnpm
command: npm install -g pnpm
working_directory: /tmp/e2e-tests/gatsby-pnpm
- run: # Install start-server-and-test
command: npm install -g start-server-and-test@^1.11.0
working_directory: /tmp/e2e-tests/gatsby-pnpm
- run: # Set project dir
command: node ~/project/packages/gatsby-dev-cli/dist/index.js --set-path-to-repo ~/project
working_directory: /tmp/e2e-tests/gatsby-pnpm
- run: # Copy over packages
command: node ~/project/packages/gatsby-dev-cli/dist/index.js --force-install --scan-once --package-manager pnpm
working_directory: /tmp/e2e-tests/gatsby-pnpm
- run:
command: pnpm build
working_directory: /tmp/e2e-tests/gatsby-pnpm
- run:
command: 'DEBUG=start-server-and-test pnpm start-server-and-test "pnpm develop 2>&1 | tee log.txt" :8000 "! cat log.txt | grep -E ''ERROR #|Require stack:''"'
working_directory: /tmp/e2e-tests/gatsby-pnpm

e2e_tests_development_runtime_with_react_18:
<<: *e2e-executor
steps:
Expand Down Expand Up @@ -606,6 +648,8 @@ workflows:
- bootstrap
- e2e_tests_pnp:
<<: *e2e-test-workflow
- e2e_tests_pnpm:
<<: *e2e-test-workflow
- e2e_tests_path-prefix:
<<: *e2e-test-workflow
- e2e_tests_visual-regression:
Expand Down
9 changes: 8 additions & 1 deletion packages/gatsby-dev-cli/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,13 @@ You typically only need to configure this once.`
.alias(`h`, `help`)
.nargs(`v`, 0)
.alias(`v`, `version`)
.describe(`v`, `Print the currently installed version of Gatsby Dev CLI`).argv
.describe(`v`, `Print the currently installed version of Gatsby Dev CLI`)
.choices(`package-manager`, [`yarn`, `pnpm`])
.default(`package-manager`, `yarn`)
.describe(
`package-manager`,
`Package manager to use for installing dependencies.`
).argv

if (argv.version) {
console.log(getVersionInfo())
Expand Down Expand Up @@ -154,4 +160,5 @@ watch(gatsbyLocation, argv.packages, {
monoRepoPackages,
packageNameToPath,
externalRegistry: argv.externalRegistry,
packageManager: argv.packageManager,
})
2 changes: 2 additions & 0 deletions packages/gatsby-dev-cli/src/local-npm-registry/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ exports.publishPackagesLocallyAndInstall = async ({
yarnWorkspaceRoot,
externalRegistry,
root,
packageManager,
}) => {
await startServer()

Expand All @@ -75,5 +76,6 @@ exports.publishPackagesLocallyAndInstall = async ({
yarnWorkspaceRoot,
newlyPublishedPackageVersions,
externalRegistry,
packageManager,
})
}
37 changes: 25 additions & 12 deletions packages/gatsby-dev-cli/src/local-npm-registry/install-packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const installPackages = async ({
yarnWorkspaceRoot,
newlyPublishedPackageVersions,
externalRegistry,
packageManager,
}) => {
console.log(
`Installing packages from local registry:\n${packagesToInstall
Expand Down Expand Up @@ -127,20 +128,32 @@ const installPackages = async ({

installCmd = [`yarn`, yarnCommands]
} else {
const yarnCommands = [
`add`,
...packagesToInstall.map(packageName => {
const packageVersion = newlyPublishedPackageVersions[packageName]
return `${packageName}@${packageVersion}`
}),
`--exact`,
]
const packageAndVersionsToInstall = packagesToInstall.map(packageName => {
const packageVersion = newlyPublishedPackageVersions[packageName]
return `${packageName}@${packageVersion}`
})

if (!externalRegistry) {
yarnCommands.push(`--registry=${registryUrl}`)
}
if (packageManager === `pnpm`) {
const pnpmCommands = [
`add`,
...packageAndVersionsToInstall,
`--save-exact`,
]

installCmd = [`yarn`, yarnCommands]
if (!externalRegistry) {
pnpmCommands.push(`--registry=${registryUrl}`)
}

installCmd = [`pnpm`, pnpmCommands]
} else {
const yarnCommands = [`add`, ...packageAndVersionsToInstall, `--exact`]

if (!externalRegistry) {
yarnCommands.push(`--registry=${registryUrl}`)
}

installCmd = [`yarn`, yarnCommands]
}
}

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const verdaccioConfig = {
title: `gatsby-dev`,
},
self_path: `./`,
logs: [{ type: `stdout`, format: `pretty-timestamped`, level: `warn` }],
logs: { type: `stdout`, format: `pretty-timestamped`, level: `warn` },
packages: {
"**": {
access: `$all`,
Expand Down
3 changes: 3 additions & 0 deletions packages/gatsby-dev-cli/src/watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ async function watch(
localPackages,
packageNameToPath,
externalRegistry,
packageManager,
}
) {
setDefaultSpawnStdio(quiet ? `ignore` : `inherit`)
Expand Down Expand Up @@ -158,6 +159,7 @@ async function watch(
yarnWorkspaceRoot,
externalRegistry,
root,
packageManager,
})
} else {
// run `yarn`
Expand Down Expand Up @@ -344,6 +346,7 @@ async function watch(
ignorePackageJSONChanges,
externalRegistry,
root,
packageManager,
})
packagesToPublish.clear()
isPublishing = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,14 @@ const APIFunctionLoader: LoaderDefinition = async function () {
const functionModule = require('${modulePath}');
const functionToExecute = preferDefault(functionModule);
const matchPath = '${matchPath}';
const { match: reachMatch } = require('@gatsbyjs/reach-router');
const { urlencoded, text, json, raw } = require('body-parser')
const multer = require('multer')
const { createConfig } = require('gatsby/dist/internal-plugins/functions/config')
const { match: reachMatch } = require('${require.resolve(
`@gatsbyjs/reach-router`
)}');
const { urlencoded, text, json, raw } = require('${require.resolve(
`body-parser`
)}')
const multer = require('${require.resolve(`multer`)}')
const { createConfig } = require('${require.resolve(`./config`)}')

function functionWrapper(req, res) {
if (matchPath) {
Expand Down
17 changes: 13 additions & 4 deletions packages/gatsby/src/internal-plugins/functions/gatsby-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { reportWebpackWarnings } from "../../utils/webpack-error-utils"
import { internalActions } from "../../redux/actions"
import { IGatsbyFunction } from "../../redux/types"
import { functionMiddlewares } from "./middleware"
import mod from "module"

const isProductionEnv = process.env.gatsby_executing_command !== `develop`

Expand Down Expand Up @@ -305,6 +306,10 @@ const createWebpackConfig = async ({
? `functions-production`
: `functions-development`

const gatsbyPluginTSRequire = mod.createRequire(
require.resolve(`gatsby-plugin-typescript`)
)

return {
entry: entries,
output: {
Expand Down Expand Up @@ -373,19 +378,23 @@ const createWebpackConfig = async ({
},
},
use: {
loader: `babel-loader`,
loader: require.resolve(`babel-loader`),
options: {
presets: [`@babel/typescript`],
presets: [
gatsbyPluginTSRequire.resolve(`@babel/preset-typescript`),
],
},
},
},
{
test: [/.js$/, /.ts$/],
exclude: /node_modules/,
use: {
loader: `babel-loader`,
loader: require.resolve(`babel-loader`),
options: {
presets: [`@babel/typescript`],
presets: [
gatsbyPluginTSRequire.resolve(`@babel/preset-typescript`),
],
},
},
},
Expand Down
86 changes: 84 additions & 2 deletions packages/gatsby/src/utils/webpack/plugins/cache-folder-resolver.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Resolver from "enhanced-resolve/lib/Resolver"
import mod from "module"

interface IRequest {
request?: string
Expand All @@ -8,17 +9,98 @@ interface IRequest {
type ProcessWithPNP = NodeJS.ProcessVersions & { pnp?: string }

/**
* To support PNP we have to make sure dependencies resolved from the .cache folder should be resolved from the gatsby package directory
* To support Yarn PNP and pnpm we have to make sure dependencies resolved from
* the .cache folder should be resolved from the gatsby package directory
* If you see error like
*
* ModuleNotFoundError: Module not found: Error: Can't resolve 'prop-types'
* in '<site-directory>/.cache'
*
* it probably means this plugin is not enabled when it should be and there
* might be need to adjust conditions for setting `this.isEnabled` in the
* constructor.
*
* It's not enabled always because of legacy behavior and to limit potential
* regressions. Might be good idea to enable it always in the future
* OR remove the need for the plugin completely by not copying `cache-dir`
* contents to `.cache` folder and instead adjust setup to use those browser/node
* html renderer runtime files directly from gatsby package
*/
export class CacheFolderResolver {
private requestsFolder: string
private isEnabled = false

constructor(requestsFolder: string) {
this.requestsFolder = requestsFolder

if ((process.versions as ProcessWithPNP).pnp) {
// Yarn PnP
this.isEnabled = true
} else if (/node_modules[/\\]\.pnpm/.test(process.env.NODE_PATH ?? ``)) {
// pnpm when executing through `pnpm` CLI
this.isEnabled = true
} else {
// pnpm when executing through regular `gatsby` CLI / `./node_modules/.bin/gatsby`
// would not set NODE_PATH, but node_modules structure would not allow to resolve
// gatsby deps from the cache folder (unless user would install same deps too)
// so we are checking if we can resolve deps from the cache folder
// this check is not limited to pnpm and other package managers could hit this path too

// Hardcoded list of gatsby deps used in gatsby browser and ssr runtimes
// instead of checking if we use Yarn PnP (via `process.versions.pnp`),
// we check if we can resolve the external deps from the cache-dir folder
// to know if we need to enable this plugin so we also cover pnpm
// It might be good idea to always enable it overall, but to limit potential
// regressions we only enable it if we are sure we need it.
const modulesToCheck = [
`prop-types`,
`lodash/isEqual`,
`mitt`,
`shallow-compare`,
`@gatsbyjs/reach-router`,
`gatsby-react-router-scroll`,
`react-server-dom-webpack`,
`gatsby-link`,
]

// test if we can resolve deps from the cache folder
let isEverythingResolvableFromCacheDir = true
const cacheDirReq = mod.createRequire(requestsFolder)
for (const cacheDirDep of modulesToCheck) {
try {
cacheDirReq.resolve(cacheDirDep)
} catch {
// something is not resolvable from the cache folder, so we should not enable this plugin
isEverythingResolvableFromCacheDir = false
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is false, could break out of the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would need 2 separate loops for checking if tested dependency is resolvable from .cache and from `gatsby package - which I think probably makes more sense than current single loop, so I will make the change to run those checks in separate loops and just break the loops on first unresolvable module

break
}
}

// test if we can resolve deps from the gatsby package
let isEverythingResolvableFromGatsbyPackage = true
for (const cacheDirDep of modulesToCheck) {
try {
require.resolve(cacheDirDep)
} catch {
// something is not resolvable from the gatsby package
isEverythingResolvableFromGatsbyPackage = false
break
}
}

// we only enable this plugin if we are unable to resolve cache-dir deps from .cache folder
// and we can resolve them from gatsby package
if (
!isEverythingResolvableFromCacheDir &&
isEverythingResolvableFromGatsbyPackage
) {
this.isEnabled = true
}
}
}

apply(resolver: Resolver): void {
if (!(process.versions as ProcessWithPNP).pnp) {
if (!this.isEnabled) {
return
}

Expand Down