Lock yarn pnp version in tests #34688

ijjk · 2022-02-22T15:53:14Z

It seems the most recent publish for yarn berry is causing a breaking change with jest-worker breaking tests. This temporarily locks the version we test against until this is resolved.

x-ref: https://github.com/vercel/next.js/runs/5290374235?check_suite_focus=true
x-ref: https://github.com/vercel/next.js/runs/5289675273?check_suite_focus=true
x-ref: https://github.com/vercel/next.js/runs/5282660046?check_suite_focus=true

styfle · 2022-02-22T16:11:20Z

test/e2e/yarn-pnp/test/utils.ts

@@ -31,7 +31,7 @@ export function runTests(example = '') {
          prev.push(`${cur}@${dependencies[cur]}`)
          return prev
        }, [] as string[])
-        return `yarn set version berry && yarn config set enableGlobalCache true && yarn config set compressionLevel 0 && yarn add ${pkgs.join(
+        return `yarn set version 3.1.1 && yarn config set enableGlobalCache true && yarn config set compressionLevel 0 && yarn add ${pkgs.join(


Suggested change

return `yarn set version 3.1.1 && yarn config set enableGlobalCache true && yarn config set compressionLevel 0 && yarn add ${pkgs.join(

return `yarn set version berry && yarn config set enableGlobalCache true && yarn config set compressionLevel 0 && yarn config set pnpEnableEsmLoader false && yarn add ${pkgs.join(

I don't think we can set this unless it's the default for users with yarn pnp, we want to test the default experience users will be having

styfle

Let's use yarn config set pnpEnableEsmLoader false since we need Node.js 16 or newer

https://yarnpkg.com/configuration/yarnrc#pnpEnableEsmLoader

ijjk · 2022-02-22T16:15:16Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
buildDuration	19s	19.1s	⚠️ +26ms
buildDurationCached	7.6s	7.6s	-20ms
nodeModulesSize	367 MB	367 MB	⚠️ +6 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.597	3.676	⚠️ +0.08
/ avg req/sec	694.93	680.13	⚠️ -14.8
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.795	1.713	-0.08
/error-in-render avg req/sec	1392.52	1459.74	+67.22

Client Bundles (main, webpack)

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	27.8 kB	27.8 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.5 kB	71.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 B	✓
dynamic-HASH.js gzip	2.57 kB	2.57 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	919 B	919 B	✓
image-HASH.js gzip	5.05 kB	5.05 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.26 kB	2.26 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.7 kB	14.7 kB	✓

Client Build Manifests

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
index.html gzip	530 B	530 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
buildDuration	23.1s	22.8s	-321ms
buildDurationCached	7.6s	7.5s	-89ms
nodeModulesSize	367 MB	367 MB	⚠️ +6 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.604	3.566	-0.04
/ avg req/sec	693.76	701.06	+7.3
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.675	1.654	-0.02
/error-in-render avg req/sec	1492.46	1511.35	+18.89

Client Bundles (main, webpack)

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.1 kB	42.1 kB	✓
main-HASH.js gzip	27.8 kB	27.8 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.6 kB	71.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.56 kB	2.56 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	911 B	911 B	✓
image-HASH.js gzip	5.08 kB	5.08 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.28 kB	2.28 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.7 kB	14.7 kB	✓

Client Build Manifests

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	ijjk/next.js test/lock-yarn-berry	Change
index.html gzip	534 B	534 B	✓
link.html gzip	547 B	547 B	✓
withRouter.html gzip	528 B	528 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: 6c41386

ijjk · 2022-02-22T16:16:29Z

Let's use yarn config set pnpEnableEsmLoader false since we need Node.js 16 or newer

As discussed in slack I don't think we should change of config in our tests for now to ensure a breaking change isn't being made in PnP which makes it appear to be Next.js fault when it's not. Let's start with locking the version here to unblock other PRs though.

arcanis · 2022-02-22T16:43:29Z

For context, the core issue isn't from Next.js, nor Yarn (nor even Jest).

Node <16.14 have a bug where, when using child_process.fork while an ESM loader is configured, signals received during the new process initialization were processed before the forked code had a chance to attach itself (ESM makes the process initialization asynchronous). As a result those events (including Jest's initialization message) were lost, and subsequent worker pool calls failed.

The problem appeared with Yarn 3.2 because it enables ESM support in more situations than before, which causes loaders to be added to the environment, and thus triggers the Node bug.

Now, considering that:

The Yarn versions are typically locked per project (so they can't suddenly start to crash)
A revert would also prevent some projects from working (ESM support is enabled when we detect it as required)
The bug doesn't come from Yarn (or Next.js) itself, but from Node
That there's a first remediation (upgrading Node to 16.14)
That there's a second remediation (enable experimental.workerThreads: true in next.config.js)

We currently don't plan to revert the ESM change. May change if we hear more problems, of course.

ijjk · 2022-02-22T17:17:52Z

@arcanis in the latest version of Yarn why is ESM being used to load Next.js or jest-worker both of these are CJS dependencies so I wouldn't think any changes related to ESM should be affecting these packages.

arcanis · 2022-02-22T18:07:03Z

Because terser and nanoid (both used in the Next.js dependency tree) define "type": "module" in their package.json, which indicates to Yarn that they need ESM support to work properly.

ijjk · 2022-02-22T19:34:52Z

Shouldn't only those specific packages be loaded with ESM by yarn and also only if loaded via import?

arcanis · 2022-02-22T19:39:03Z

No; ESM packages can be imported from commonjs code using the import() function, which requires the loader to already be in the environment (Node doesn't support adding loaders once it has already started, they all need to be there upfront).

ijjk · 2022-02-22T19:46:59Z

So just having the ESM loader configured and not even necessarily being used for jest-worker or Next.js specific code causes nodejs/node#41221 to be a bug with yarn on Node.js versions < 16.14?

arcanis · 2022-02-22T19:53:52Z

As I mentioned it's not a bug in Yarn; we use the APIs provided. But for the other points yes: having any ESM loader set (PnP or otherwise, such as esbuild-node-loader) will cause jest-worker to fail in Node <16.14 even if the loader isn't actually called.

ijjk · 2022-02-22T20:14:11Z

If this isn't specific to Yarn berry can we reproduce it with npm or Yarn v1 leveraging the built-in node ESM handling? Sorry if this is a dumb question, still trying to figure out how we can prevent users from ending up in this situation since it can be very confusing to understand what's causing the issue if it seems the process is just hanging and doesn't seem evident what can be the cause since no warnings/errors are displayed.

arcanis · 2022-02-23T09:32:46Z

Yes; it's a little more awkward due to another Node bug that the Yarn loader workarounds (having a loader configured currently forces the entry point to be evaluated in ESM mode, thus preventing the Next binary to run at all), but the same can be reproduced with npm with a little help:

next.mjs

import {createRequire} from 'module';

createRequire(import.meta.url)('next/dist/bin/next');

command line

node --experimental-loader ./path-to-an-empty-file.mjs ./next.mjs build

merceyz · 2022-02-23T09:36:35Z

It's possible to workaround the issue in userland by having the child process send a "ready" message before the parent starts sending messages to it.
Ref jestjs/jest#12060

Lock yarn pnp version in tests

6c41386

ijjk added the created-by: Next.js team PRs by the Next.js team label Feb 22, 2022

styfle approved these changes Feb 22, 2022

View reviewed changes

ijjk marked this pull request as ready for review February 22, 2022 15:58

ijjk requested review from timneutkens, shuding and huozhi as code owners February 22, 2022 15:58

styfle reviewed Feb 22, 2022

View reviewed changes

styfle requested changes Feb 22, 2022

View reviewed changes

ijjk requested a review from styfle February 22, 2022 16:16

styfle approved these changes Feb 22, 2022

View reviewed changes

ijjk merged commit 5f1536b into vercel:canary Feb 22, 2022

ijjk deleted the test/lock-yarn-berry branch February 22, 2022 16:29

This was referenced Feb 25, 2022

[BUG] Stuck at yarn build when using latest yarn pnp #34794

Closed

Add warning for Yarn PnP v3 with Node.js < 16.14 #34818

Merged

arcanis mentioned this pull request Feb 27, 2022

[Bug?]: Timeouts when using PnP with ESM on Node <=16.14 yarnpkg/berry#4157

Closed

1 task

kurone-ab mentioned this pull request Mar 14, 2022

@next/next/no-server-import-in-page error occurs in nested route #35298

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock yarn pnp version in tests #34688

Lock yarn pnp version in tests #34688

ijjk commented Feb 22, 2022 •

edited

styfle Feb 22, 2022

ijjk Feb 22, 2022

styfle left a comment

ijjk commented Feb 22, 2022

ijjk commented Feb 22, 2022

arcanis commented Feb 22, 2022 •

edited

ijjk commented Feb 22, 2022 •

edited

arcanis commented Feb 22, 2022 •

edited

ijjk commented Feb 22, 2022 •

edited

arcanis commented Feb 22, 2022

ijjk commented Feb 22, 2022

arcanis commented Feb 22, 2022

ijjk commented Feb 22, 2022 •

edited

arcanis commented Feb 23, 2022 •

edited

merceyz commented Feb 23, 2022 •

edited

	return `yarn set version 3.1.1 && yarn config set enableGlobalCache true && yarn config set compressionLevel 0 && yarn add ${pkgs.join(
	return `yarn set version berry && yarn config set enableGlobalCache true && yarn config set compressionLevel 0 && yarn config set pnpEnableEsmLoader false && yarn add ${pkgs.join(

Lock yarn pnp version in tests #34688

Lock yarn pnp version in tests #34688

Conversation

ijjk commented Feb 22, 2022 • edited

styfle Feb 22, 2022

Choose a reason for hiding this comment

ijjk Feb 22, 2022

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

ijjk commented Feb 22, 2022

Stats from current PR

ijjk commented Feb 22, 2022

arcanis commented Feb 22, 2022 • edited

ijjk commented Feb 22, 2022 • edited

arcanis commented Feb 22, 2022 • edited

ijjk commented Feb 22, 2022 • edited

arcanis commented Feb 22, 2022

ijjk commented Feb 22, 2022

arcanis commented Feb 22, 2022

ijjk commented Feb 22, 2022 • edited

arcanis commented Feb 23, 2022 • edited

merceyz commented Feb 23, 2022 • edited

ijjk commented Feb 22, 2022 •

edited

arcanis commented Feb 22, 2022 •

edited

ijjk commented Feb 22, 2022 •

edited

arcanis commented Feb 22, 2022 •

edited

ijjk commented Feb 22, 2022 •

edited

ijjk commented Feb 22, 2022 •

edited

arcanis commented Feb 23, 2022 •

edited

merceyz commented Feb 23, 2022 •

edited