-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Add edge polyfills for nextjs-13 in dev mode #17488
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
a945362
to
56b2b9f
Compare
56b2b9f
to
46aaac9
Compare
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.
Nice work, just quick comments/questions
DefinePlugin: class {} as any, | ||
ProvidePlugin: class { | ||
constructor(public definitions: Record<string, any>) {} | ||
} as any, |
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.
Nit: Can we use a more precise type? or type this with unknown instead of any please?
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.
These are just test fixtures, I don't think it's necessary to be more precise here.
addOtelWarningIgnoreRule(newConfig); | ||
|
||
// Add edge runtime polyfills when building for edge in dev mode | ||
if (runtime === 'edge' && isDev) { |
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.
This will run for all next versions with dev mode correct? is this intended?
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.
Right, I'll have a look if I can scope this to 13 only.
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.
Updated to only take 13 into account.
// Ensure performance global is available | ||
if (typeof globalThis !== 'undefined' && globalThis.performance === undefined) { | ||
globalThis.performance = { | ||
timeOrigin: 0, |
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.
Maybe do:
const timeOrigin = Date.now();
return {
timeOrigin,
now: () => Date.now() - timeOrigin,
};
I think anyone doing timing assertions in dev might see different values
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.
Right, updated.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
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.
thanks!
This issue is only observed for Nextjs 13 and specifically for apps that don't use the app router. Similarly to what we do in vercel-edge, we polyfill
performance
. This is only done for dev mode as it's not an issue otherwise.Note: I tried to reproduce this via our existing e2e test apps but they're all using the app router so it's not manifesting there.
Closes: #17343