Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 02b9d8f. Configure here.
size-limit report 📦
|
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Overall LGTM, just got one bigger concern.
| name: 'save blogposts', | ||
| status: 'ok', | ||
| is_segment: false, | ||
| attributes: expect.objectContaining({ |
There was a problem hiding this comment.
l: Maybe you could apply our /write-tests skill on these tests, they should actualy remove the expect.objectContaining assertions.
| // startup or on first connect. | ||
| const UNWRAPPED_DRIVER_DEPENDENCIES = new Set([ | ||
| // mongoose | ||
| 'kareem', |
There was a problem hiding this comment.
q/m: If one of these libs now add/remove any dependency then it would fail again right? Unless we ship a new version. Is there any chance we can keep this up to date without us maintaining this list manually? Since this is inside the bundler we could maybe get the dependency list somehow?
There was a problem hiding this comment.
The list is gone now.
But these failures probably come from @rollup/plugin-commonjs: their check probably stopped working on Node 23, because Node now adds an extra 'module.exports' key to imported CommonJS modules (nodejs/node#53848).
So instead of listing every affected package, sentryCommonJSInteropPlugin fixes that one line during the build. Everything then behaves exactly like it did on Node 22.
I also added a PR in the rollup commonjs plugin: rollup/plugins#2027
This branch has not been deployed
2 participants
Since v11, build-time instrumentation force-inlines the instrumented drivers (mongoose, ioredis, mysql, ...) into the Nitro bundle while their CommonJS dependencies stay external. Since Node 23, CJS namespaces carry a
module.exportskey next todefault(nodejs/node#53848), so Rollup's'auto'interop no longer unwraps those requires and the drivers crash at startup (mquery is not a constructor).No single interop mode (a
requireReturnsDefaultvalue) fixes this and works for all externals.debugneeds the namespace, whilemqueryandlodash.defaultsneedmodule.exports. So we unwrap builtins plus a known list and keep'auto'for the rest.Fixes #24775
Fixes #24786