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
Use require.resolve to detect "framework" packages (fix pnpm) #21048
Use require.resolve to detect "framework" packages (fix pnpm) #21048
Conversation
This comment has been minimized.
This comment has been minimized.
f4a47b4
to
eb0742d
Compare
This comment has been minimized.
This comment has been minimized.
eb0742d
to
3740547
Compare
This comment has been minimized.
This comment has been minimized.
I've added integration tests for the bug and verified (locally) that the tests fail without 3740547. Once CI passes, this is ready for review from my end. Feedback appreciated. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Try to avoid 'EBUSY: resource busy or locked' on Windows when removing temp directory
This comment has been minimized.
This comment has been minimized.
Try to avoid 'EBUSY: resource busy or locked' on Windows when removing temp directory
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It's no longer needed because 'next' no longer declares a direct dep on 'prop-types' since c7e2a1d
47ede67
to
4450a12
Compare
This comment has been minimized.
This comment has been minimized.
@sokra I've fixed the test failures. The error in the next-swc job |
This comment has been minimized.
This comment has been minimized.
- Upgrade to react@17 for next@11 compat - Add .babelrc to disable SWC - Add @next/polyfill-nomodule dep
This comment has been minimized.
This comment has been minimized.
Bump 👍 As I mentioned it here, the --shamefully-hoist workaround stopped working for Next.js 12 Serverless Functions in Vercel #16471 (comment) EDIT: It seems like the --shamefully-hoist workaround was fixed for Next.js 12.0.3, maybe related to #30786 🎉 |
// 'node_modules/react-scroll-parallax/node_modules/prop-types' are not included. | ||
const topLevelFrameworkPaths = [ | ||
getPackagePath('react', dir), | ||
getPackagePath('react-dom', dir), |
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.
prop-types seem to be missing here?
It was part of the original regexp
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.
Also react
and react-dom
depend on object-assign
so that might also need to be added here, even if it wasn't part of the regexp before.
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.
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.
Added
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.
For what it's worth, this error is raised when getPackagePath('prop-types', dir)
is added to the array:
> pnpm-app@1.0.0 build /private/var/folders/zj/25m6hvw14q5fgtm87smrxsbc0000gn/T/hobwb9bsxk9/app
> next build
info - Checking validity of types
warn - No ESLint configuration detected. Run next lint to begin setup
info - Disabled SWC as replacement for Babel because of custom Babel configuration ".babelrc" https://nextjs.org/docs/messages/swc-disabled
> Build error occurred
Error: Cannot find module 'prop-types/package.json'
Require stack:
- /private/var/folders/zj/25m6hvw14q5fgtm87smrxsbc0000gn/T/hobwb9bsxk9/app/node_modules/.pnpm/local++var+folders+zj+25m6hvw14q5fgtm87smrxsbc0000_05da36a7aef9195a59c6f2b92d013fed/node_modules/next/dist/build/webpack-config.js
- /private/var/folders/zj/25m6hvw14q5fgtm87smrxsbc0000gn/T/hobwb9bsxk9/app/node_modules/.pnpm/local++var+folders+zj+25m6hvw14q5fgtm87smrxsbc0000_05da36a7aef9195a59c6f2b92d013fed/node_modules/next/dist/build/index.js
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Should fix the 'pid must be a number' error when `killProcess(appProcess.pid)` is called.
This comment has been minimized.
This comment has been minimized.
Failing test suitesCommit: c097bb2 test/integration/pnpm-support/test/index.test.js
Expand output● pnpm support › should execute client-side JS on each page
|
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
buildDuration | 21.9s | 21.6s | -289ms |
buildDurationCached | 4.5s | 4.4s | -133ms |
nodeModulesSize | 332 MB | 332 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.578 | 3.577 | 0 |
/ avg req/sec | 698.79 | 698.93 | +0.14 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.739 | 1.678 | -0.06 |
/error-in-render avg req/sec | 1437.93 | 1489.9 | +51.97 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
main-HASH.js gzip | 28.1 kB | 28.1 kB | ✓ |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 71.9 kB | 71.9 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.23 kB | 1.23 kB | ✓ |
_error-HASH.js gzip | 194 B | 194 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 2.38 kB | 2.38 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 635 B | 635 B | ✓ |
image-HASH.js gzip | 4.44 kB | 4.44 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 1.87 kB | 1.87 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 | 13.1 kB | 13.1 kB | ✓ |
Client Build Manifests
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
_buildManifest.js gzip | 460 B | 460 B | ✓ |
Overall change | 460 B | 460 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
index.html gzip | 523 B | 523 B | ✓ |
link.html gzip | 536 B | 536 B | ✓ |
withRouter.html gzip | 518 B | 518 B | ✓ |
Overall change | 1.58 kB | 1.58 kB | ✓ |
Default Build with SWC
General Overall increase ⚠️
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
buildDuration | 23.5s | 22.9s | -687ms |
buildDurationCached | 4.4s | 4.1s | -295ms |
nodeModulesSize | 332 MB | 332 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.575 | 3.43 | -0.15 |
/ avg req/sec | 699.33 | 728.8 | +29.47 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.65 | 1.677 | |
/error-in-render avg req/sec | 1515.43 | 1490.49 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 28.2 kB | 28.2 kB | ✓ |
webpack-HASH.js gzip | 1.43 kB | 1.43 kB | ✓ |
Overall change | 72.1 kB | 72.1 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.22 kB | 1.22 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.38 kB | 2.38 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 622 B | 622 B | ✓ |
image-HASH.js gzip | 4.46 kB | 4.46 kB | ✓ |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 1.91 kB | 1.91 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 | 13.1 kB | 13.1 kB | ✓ |
Client Build Manifests
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | elliottsj/next.js split-chunks-framework-pnpm-fix | Change | |
---|---|---|---|
index.html gzip | 523 B | 523 B | ✓ |
link.html gzip | 535 B | 535 B | ✓ |
withRouter.html gzip | 517 B | 517 B | ✓ |
Overall change | 1.57 kB | 1.57 kB | ✓ |
@sokra @timneutkens tests now fixed |
Fix #20884
The previous regex was not compatible with pnpm's module paths:
e.g.
yarn
pnpm
This meant that the
static/chunks/framework.[hash].js
chunk was not added to the build manifest, and never rendered as a<link href="..." as="script">
, so "react" / "react-dom" would never be loaded and we would never be able to execute the bundle's entry point.By replacing the regex with
require.resolve()
to check whether modules should be moved to the "framework" chunk, we're using Node.js's module resolution instead of relying on a regex which implicitly assumes the folder structure produced by npm/yarn.Tests pending. I'm thinking of modifying the existing pnpm integration test to verify that pages execute JS under a particular module configuration (like nextjs-pnpm-issue-2021-01-07), but ideally using something even more minimal than adding a dependency on react-modal. I'll look into this more soon, feedback appreciated.I've added integration tests for the bug and verified (locally) that the tests fail without 3740547.