Fix node timers #619
Fix node timers #619
Conversation
🦋 Changeset detectedLatest commit: 507ca50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
|
Thanks for the fix @conico974 FYI this was discussed internally and we decided that mixing APIs would not be a problem (documented here). I'll raise the issue again internally. One possible fix would be to update unenv (with the caveat that external modules would not be polyfilled). For now I'm +1 for this PR. |
| @@ -144,6 +144,10 @@ export async function bundleServer(buildOpts: BuildOptions): Promise<void> { | |||
| // This define should be safe to use for Next 14.2+, earlier versions (13.5 and less) will cause trouble | |||
| "process.env.__NEXT_EXPERIMENTAL_REACT": `${needsExperimentalReact(nextConfig)}`, | |||
| }, | |||
| banner: { | |||
There was a problem hiding this comment.
We could move the code to init.ts#initRuntime
There was a problem hiding this comment.
Already tried that, putting the import there does not work.
And as said in the comment we can't just assign to globalThis without causing a Maximum call stack error.
We could also polyfill it in initRuntime, which is what i initially did in d7ce16b
There was a problem hiding this comment.
Sorry I miss the comment...
Would Object.assign() work instead of direct assignment?
There might be some interaction with unenv, I can take a look at that
There was a problem hiding this comment.
Already tried that as well.
I think that the current implementation in node:timers internally use globalThis.setInterval which make it call itself
There was a problem hiding this comment.
It is expected that the global timers (i.e.
setInterval,setTimeout) in Node returns aNodejs.Timeoutinstead of a number. On cloudflare workers they return a number.This can cause some unexpected crash like in #587, where sentry tries to call
unref()on the created interval.This PR fix #587
The only downsides to doing that is that in the middleware we'll have access to the Node version as well, but i don't think this is too big of an issue