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

Resolve to real path before checking for path inequality #17279

Merged
merged 7 commits into from Oct 14, 2020

Conversation

remorses
Copy link
Contributor

Currently the nextjs server webpack config bundles all the modules that are symlinked because of the following line

if (baseRes !== res) {

I am resolving to the real path to handle symlinks correctly without changing the resolution algorithm

Fixes pnpm and will probably make the server bundle lighter when using many symlinked packages
Fixes #16988

@ijjk
Copy link
Member

ijjk commented Sep 22, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 14.2s 14.4s ⚠️ +164ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +385 B
Page Load Tests Overall increase ✓
vercel/next.js canary remorses/next.js fix-pnpm Change
/ failed reqs 0 0
/ total time (seconds) 2.682 2.625 -0.06
/ avg req/sec 932 952.53 +20.53
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.574 1.569 -0.01
/error-in-render avg req/sec 1588.78 1593.22 +4.44
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-772ea11..1d22.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-911f3f8..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes
vercel/next.js canary remorses/next.js fix-pnpm Change
index.html gzip 1 kB 1 kB
link.html gzip 1.01 kB 1.01 kB
withRouter.html gzip 997 B 997 B
Overall change 3.01 kB 3.01 kB

Serverless Mode
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 16.5s 15.9s -638ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +385 B
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-772ea11..1d22.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-911f3f8..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary remorses/next.js fix-pnpm Change
_error.js 1.04 MB 1.04 MB
404.html 4.34 kB 4.34 kB
hooks.html 3.92 kB 3.92 kB
index.js 1.04 MB 1.04 MB
link.js 1.08 MB 1.08 MB
routerDirect.js 1.08 MB 1.08 MB
withRouter.js 1.08 MB 1.08 MB
Overall change 5.31 MB 5.31 MB
Commit: 3caacd5

@ijjk
Copy link
Member

ijjk commented Sep 22, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 14.4s 13.9s -560ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +398 B
Page Load Tests Overall increase ✓
vercel/next.js canary remorses/next.js fix-pnpm Change
/ failed reqs 0 0
/ total time (seconds) 2.604 2.585 -0.02
/ avg req/sec 960.22 967.29 +7.07
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.677 1.624 -0.05
/error-in-render avg req/sec 1490.99 1539.01 +48.02
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-772ea11..1d22.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-911f3f8..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes
vercel/next.js canary remorses/next.js fix-pnpm Change
index.html gzip 1 kB 1 kB
link.html gzip 1.01 kB 1.01 kB
withRouter.html gzip 997 B 997 B
Overall change 3.01 kB 3.01 kB

Serverless Mode
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 15.4s 16s ⚠️ +557ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +398 B
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-772ea11..1d22.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-911f3f8..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary remorses/next.js fix-pnpm Change
_error.js 1.04 MB 1.04 MB
404.html 4.34 kB 4.34 kB
hooks.html 3.92 kB 3.92 kB
index.js 1.04 MB 1.04 MB
link.js 1.08 MB 1.08 MB
routerDirect.js 1.08 MB 1.08 MB
withRouter.js 1.08 MB 1.08 MB
Overall change 5.31 MB 5.31 MB
Commit: be7cc80

@Janpot
Copy link
Contributor

Janpot commented Sep 22, 2020

@remorses Would it be possible to write an integration test for this? Is it possible to simulate the faulty behavior?

Copy link
Member

@lfades lfades left a comment

Choose a reason for hiding this comment

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

Blocking as per @Janpot comment, we need an integration test to make sure this works and don't regress in the future.

@remorses
Copy link
Contributor Author

I added the integration test but it is quite slow, do you have a way to run that specific test only on Ci and not locally?

@ijjk
Copy link
Member

ijjk commented Sep 28, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 12.1s 12.2s ⚠️ +154ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +398 B
Page Load Tests Overall increase ✓
vercel/next.js canary remorses/next.js fix-pnpm Change
/ failed reqs 0 0
/ total time (seconds) 2.227 2.202 -0.02
/ avg req/sec 1122.59 1135.37 +12.78
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.179 1.167 -0.01
/error-in-render avg req/sec 2120.31 2142.34 +22.03
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-e3dd5fc..bc67.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-f4d7a25..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes
vercel/next.js canary remorses/next.js fix-pnpm Change
index.html gzip 1 kB 1 kB
link.html gzip 1.01 kB 1.01 kB
withRouter.html gzip 996 B 996 B
Overall change 3.01 kB 3.01 kB

Serverless Mode
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 13.7s 13.4s -374ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +398 B
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-e3dd5fc..bc67.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-f4d7a25..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary remorses/next.js fix-pnpm Change
_error.js 1.05 MB 1.05 MB
404.html 4.34 kB 4.34 kB
hooks.html 3.92 kB 3.92 kB
index.js 1.05 MB 1.05 MB
link.js 1.1 MB 1.1 MB
routerDirect.js 1.09 MB 1.09 MB
withRouter.js 1.09 MB 1.09 MB
Overall change 5.4 MB 5.4 MB
Commit: 7e948d8

@ijjk
Copy link
Member

ijjk commented Sep 28, 2020

Failing test suites

Commit: 7e948d8

test/package-managers-tests/basic-pnpm/test/index.test.js

  • pnpm installs

  • nextjs builds with pnpm

Expand output

● pnpm installs

Command failed: npm i -g pnpm

  27 | 
  28 | const exec = (cmd) => {
> 29 |   return execSync(cmd, {
     |          ^
  30 |     env: process.env,
  31 |     shell: true,
  32 |     stdio: 'inherit',

  at exec (package-managers-tests/basic-pnpm/test/index.test.js:29:10)
  at package-managers-tests/basic-pnpm/test/index.test.js:9:3

● nextjs builds with pnpm

Command failed: npm i -g pnpm

  27 | 
  28 | const exec = (cmd) => {
> 29 |   return execSync(cmd, {
     |          ^
  30 |     env: process.env,
  31 |     shell: true,
  32 |     stdio: 'inherit',

  at exec (package-managers-tests/basic-pnpm/test/index.test.js:29:10)
  at package-managers-tests/basic-pnpm/test/index.test.js:9:3

@lfades
Copy link
Member

lfades commented Sep 28, 2020

@remorses We have some instructions for running only a subset of tests here: https://github.com/vercel/next.js/blob/canary/contributing.md#to-run-tests

@ijjk
Copy link
Member

ijjk commented Sep 28, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 13.1s 13.4s ⚠️ +305ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +398 B
Page Load Tests Overall increase ✓
vercel/next.js canary remorses/next.js fix-pnpm Change
/ failed reqs 0 0
/ total time (seconds) 2.508 2.471 -0.04
/ avg req/sec 996.72 1011.93 +15.21
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.481 1.407 -0.07
/error-in-render avg req/sec 1688.04 1777.34 +89.3
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-e3dd5fc..bc67.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-f4d7a25..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes
vercel/next.js canary remorses/next.js fix-pnpm Change
index.html gzip 1 kB 1 kB
link.html gzip 1.01 kB 1.01 kB
withRouter.html gzip 996 B 996 B
Overall change 3.01 kB 3.01 kB

Serverless Mode
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 14.5s 14.5s ⚠️ +27ms
nodeModulesSize 62.9 MB 62.9 MB ⚠️ +398 B
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-e3dd5fc..bc67.js gzip 7.16 kB 7.16 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-f4d7a25..dule.js gzip 6.23 kB 6.23 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary remorses/next.js fix-pnpm Change
_error.js 1.05 MB 1.05 MB
404.html 4.34 kB 4.34 kB
hooks.html 3.92 kB 3.92 kB
index.js 1.05 MB 1.05 MB
link.js 1.1 MB 1.1 MB
routerDirect.js 1.09 MB 1.09 MB
withRouter.js 1.09 MB 1.09 MB
Overall change 5.4 MB 5.4 MB
Commit: 15a991d

test('pnpm installs', async () => {
const pnpm = getStdout('yarn bin pnpm')
exec(pnpm + ' init -y')
exec(pnpm + ' add next react react-dom')
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, these pnpm tests should execute somewhere isolated from the monorepo's own node_modules. This helps prevent issues where a module is mistakenly imported from the monorepo node_modules instead of pnpm's node_modules, leading to false test results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no way to do that other than modifying the CI script, making it even more brittle

Furthermore, the test installs the dependencies before starting, the installed node_modules will always have precedence over the root node_modules

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair.

I'm more worried about cases where a dependency is used by the Next.js app under test, but is missing from the pnpm node_modules tree.

For example, if we forget to install react or another peer dependency, then it will instead be imported from the monorepo node_modules.

A more likely scenario is that a dependency is mistakenly omitted from the next package's dependencies list, e.g. #8344 (this example is for "next-server", but the same could apply to "next"). We want this to cause the pnpm integration test to fail, but if it's not isolated from the monorepo node_modules, then the test will pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no way to do that other than modifying the CI script

Is it possible to have this test create a temp folder outside of the next.js project tree, copy over package.json and pages and run the pnpm commands on that folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i can do that

@ijjk
Copy link
Member

ijjk commented Oct 7, 2020

Stats from current PR

Default Server Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 12.7s 12.8s ⚠️ +92ms
nodeModulesSize 63.2 MB 63.2 MB ⚠️ +398 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
/ failed reqs 0 0
/ total time (seconds) 2.295 2.339 ⚠️ +0.04
/ avg req/sec 1089.42 1068.98 ⚠️ -20.44
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.191 1.226 ⚠️ +0.03
/error-in-render avg req/sec 2099.68 2039.65 ⚠️ -60.03
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-e5f5aee..31e1.js gzip 7.17 kB 7.17 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-ff4dea7..dule.js gzip 6.24 kB 6.24 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes
vercel/next.js canary remorses/next.js fix-pnpm Change
index.html gzip 1 kB 1 kB
link.html gzip 1.01 kB 1.01 kB
withRouter.html gzip 996 B 996 B
Overall change 3.01 kB 3.01 kB

Serverless Mode
General Overall increase ⚠️
vercel/next.js canary remorses/next.js fix-pnpm Change
buildDuration 14.1s 14s -124ms
nodeModulesSize 63.2 MB 63.2 MB ⚠️ +398 B
Client Bundles (main, webpack, commons)
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..9339.js gzip 10.9 kB 10.9 kB
framework.HASH.js gzip 39 kB 39 kB
main-e5f5aee..31e1.js gzip 7.17 kB 7.17 kB
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.8 kB 57.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
677f882d2ed8..dule.js gzip 6.77 kB 6.77 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-ff4dea7..dule.js gzip 6.24 kB 6.24 kB
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52.7 kB 52.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary remorses/next.js fix-pnpm Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-ed1b0..8fbd.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-d2344ce..8b36.js gzip 1.3 kB 1.3 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.71 kB 7.71 kB
Client Pages Modern
vercel/next.js canary remorses/next.js fix-pnpm Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-4469a..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-f8c0daf..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary remorses/next.js fix-pnpm Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary remorses/next.js fix-pnpm Change
_error.js 1.05 MB 1.05 MB
404.html 4.34 kB 4.34 kB
hooks.html 3.92 kB 3.92 kB
index.js 1.05 MB 1.05 MB
link.js 1.1 MB 1.1 MB
routerDirect.js 1.09 MB 1.09 MB
withRouter.js 1.09 MB 1.09 MB
Overall change 5.4 MB 5.4 MB
Commit: 05b04a6

@sdalonzo
Copy link

sdalonzo commented Oct 8, 2020

I'm looking forward to this getting merged and published! Our next apps are pinned to 9.4.4 in our Rush monorepo due to this, and we really need basePath to support our GKE strategy.

@theprobugmaker
Copy link

@sdalonzo [off-topic] I would love to know how you guys setup your deployment (CI/CD) for a monorepo, it's really complicated to find resources about that, most of the tutorials/articles are about development and 99,9% of them doesn't enter on the aspect of deployment.

I'm looking forward to seeing this getting merged as well, I've been following a few issues and prs regarding this for a while.

@sdalonzo
Copy link

@sdalonzo [off-topic] I would love to know how you guys setup your deployment (CI/CD) for a monorepo, it's really complicated to find resources about that, most of the tutorials/articles are about development and 99,9% of them doesn't enter on the aspect of deployment.

I'm looking forward to seeing this getting merged as well, I've been following a few issues and prs regarding this for a while.

I've been considering some blog posts on the topic. Without hijacking the thread here, I'd certainly be willing to share some things we learned on our CI/CD journey.

@theprobugmaker
Copy link

@sdalonzo [off-topic] I would love to know how you guys setup your deployment (CI/CD) for a monorepo, it's really complicated to find resources about that, most of the tutorials/articles are about development and 99,9% of them doesn't enter on the aspect of deployment.
I'm looking forward to seeing this getting merged as well, I've been following a few issues and prs regarding this for a while.

I've been considering some blog posts on the topic. Without hijacking the thread here, I'd certainly be willing to share some things we learned on our CI/CD journey.

It would be great. If possible I would like to have your e-mail or Twitter so I can ask you a few questions. Thank you.

.trim()
}

async function useLocalNextjs() {
Copy link
Member

Choose a reason for hiding this comment

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

This function looks incorrect as it only considers packages/next but Next.js has a dependency of other packages in the monorepo as well which would not be copied, so this would break eventually.

Copy link
Member

Choose a reason for hiding this comment

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

Can you fix that in a follow-up PR as I'll merge this one.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

This change looks good. Sorry for taking a bit longer than usual to merge this PR. Last time the externals calculation was changed it accidentally bundled too much code causing performance issues so I had to verify this change works as intended.

@timneutkens timneutkens merged commit 5c4d0bd into vercel:canary Oct 14, 2020
@sdalonzo
Copy link

@sdalonzo [off-topic] I would love to know how you guys setup your deployment (CI/CD) for a monorepo, it's really complicated to find resources about that, most of the tutorials/articles are about development and 99,9% of them doesn't enter on the aspect of deployment.
I'm looking forward to seeing this getting merged as well, I've been following a few issues and prs regarding this for a while.

I've been considering some blog posts on the topic. Without hijacking the thread here, I'd certainly be willing to share some things we learned on our CI/CD journey.

It would be great. If possible I would like to have your e-mail or Twitter so I can ask you a few questions. Thank you.

@zefexdeveloper https://twitter.com/steve_dalonzo, I can provide email if you DM on Twitter. FYI, completing moving to a new house with a toddler, so apologies if responses are not prompt.

@timneutkens Thanks for your time on this. What is typically the turnaround between merge to canary and release?

@flybayer
Copy link
Contributor

@sdalonzo this is already released in v9.5.6-canary.0

@theprobugmaker
Copy link

@sdalonzo You just need to yarn add next@canary (if I'm not mistaken)

@sdalonzo
Copy link

@sdalonzo You just need to yarn add next@canary (if I'm not mistaken)

@zefexdeveloper you must mean rush add -p next@canary ;)

@theprobugmaker
Copy link

@sdalonzo haha, rush/pnpm FTW

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9.5.x build fails if node_modules folder is a symlink and imported packages use hooks
9 participants