-
Notifications
You must be signed in to change notification settings - Fork 91
fix: handle node middleware bundling when pnpm is used #3126
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
Conversation
📊 Package size report 0.01%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
4e95752
to
ec69eeb
Compare
|
||
const content = await readFile(srcPath, 'utf8') | ||
const stats = await stat(srcPath) | ||
if (stats.isDirectory()) { |
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.
Some notes here:
While this make it work, it's actually not ideal, but the problem itself is either with Next.js or NFT itself where they don't track to individual modules and instead just output package root.
For example - here's list of node_modules/next
files listed if npm
is used:
"../../node_modules/next/dist/client/components/app-router-headers.js",
"../../node_modules/next/dist/compiled/@opentelemetry/api/index.js",
"../../node_modules/next/dist/compiled/@opentelemetry/api/package.json",
"../../node_modules/next/dist/compiled/jsonwebtoken/index.js",
"../../node_modules/next/dist/compiled/jsonwebtoken/package.json",
"../../node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js",
"../../node_modules/next/dist/lib/client-and-server-references.js",
"../../node_modules/next/dist/lib/constants.js",
"../../node_modules/next/dist/lib/interop-default.js",
"../../node_modules/next/dist/lib/is-error.js",
"../../node_modules/next/dist/lib/semver-noop.js",
"../../node_modules/next/dist/server/app-render/action-async-storage-instance.js",
"../../node_modules/next/dist/server/app-render/action-async-storage.external.js",
"../../node_modules/next/dist/server/app-render/after-task-async-storage-instance.js",
"../../node_modules/next/dist/server/app-render/after-task-async-storage.external.js",
"../../node_modules/next/dist/server/app-render/async-local-storage.js",
"../../node_modules/next/dist/server/app-render/cache-signal.js",
"../../node_modules/next/dist/server/app-render/module-loading/track-module-loading.external.js",
"../../node_modules/next/dist/server/app-render/module-loading/track-module-loading.instance.js",
"../../node_modules/next/dist/server/app-render/work-async-storage-instance.js",
"../../node_modules/next/dist/server/app-render/work-async-storage.external.js",
"../../node_modules/next/dist/server/app-render/work-unit-async-storage-instance.js",
"../../node_modules/next/dist/server/app-render/work-unit-async-storage.external.js",
"../../node_modules/next/dist/server/lib/cache-handlers/default.external.js",
"../../node_modules/next/dist/server/lib/incremental-cache/memory-cache.external.js",
"../../node_modules/next/dist/server/lib/incremental-cache/shared-cache-controls.external.js",
"../../node_modules/next/dist/server/lib/incremental-cache/tags-manifest.external.js",
"../../node_modules/next/dist/server/lib/lru-cache.js",
"../../node_modules/next/dist/server/lib/router-utils/instrumentation-globals.external.js",
"../../node_modules/next/dist/server/lib/router-utils/instrumentation-node-extensions.js",
"../../node_modules/next/dist/server/lib/trace/constants.js",
"../../node_modules/next/dist/server/lib/trace/tracer.js",
"../../node_modules/next/dist/server/load-manifest.external.js",
"../../node_modules/next/dist/server/response-cache/types.js",
"../../node_modules/next/dist/shared/lib/deep-freeze.js",
"../../node_modules/next/dist/shared/lib/invariant-error.js",
"../../node_modules/next/dist/shared/lib/is-plain-object.js",
"../../node_modules/next/dist/shared/lib/is-thenable.js",
"../../node_modules/next/dist/shared/lib/server-reference-info.js",
"../../node_modules/next/package.json",
While this is what happens when pnpm
is used:
"../../node_modules/next"
With npm
, this is subset of all the files in next
package, but with pnpm
case we will "over-bundle", but only alternative here we can do is to intentionally not support pnpm
case with more meaningful error message than current non-descriptive error message (current non-descriptive error is reported in #3114 ).
Finally - ideally the issue is handled upstream, so traced files are actually files and that pnpm
usage is not not resulting in dirs being reported. This is mostly work-arounding this issue
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.
LGTM if we find this is necessary/worthwhile, but I think this may be covered by our documented pnpm limitation/workaround: https://opennext.js.org/netlify#pnpm-support (?)
Unfortunately using the hoisting flag is not really helping here. Even with those used, pnpm is still producing symlinks under |
Description
nft when pnpm is used seems to report symlinked directories instead of actual files in ...
files
output. This will look to add handling for that as currently we assume that "files" are ... filesTests
Adding test case for node middleware + pnpm
Relevant links (GitHub issues, etc.) or a picture of cute animal
Fixes #3114