Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
b1bab86 to
b6337c2
Compare
b6337c2 to
b8024d5
Compare
| "ts-node": "^10.9.2" | ||
| }, | ||
| "volta": { | ||
| "node": "18.20.8", |
There was a problem hiding this comment.
q: Are there any downsides to upgrade to v22 here? In the JS SDK we use v20, and I'm not sure if we can upgrade that easily (maybe we can)
There was a problem hiding this comment.
I can't see any downside! We just have to be aware that every CI job that doesn't override the Node version will use this.
|
Also for the record. I'm fine with a RC of rolldown being in here. If there are no issues with it ofc - but I'm not a pro in rolldown either :) |
| @@ -0,0 +1,22 @@ | |||
| import modulePackage from "module"; | |||
There was a problem hiding this comment.
Bug: The Rollup config for bundler-plugin-core doesn't mark Node.js built-in modules as external, which will cause them to be incorrectly bundled and lead to runtime errors.
Severity: HIGH
Suggested Fix
Update the external array in packages/bundler-plugin-core/rollup.config.mjs to include Node.js built-in modules. Change external: Object.keys(packageJson.dependencies) to external: [...Object.keys(packageJson.dependencies), ...modulePackage.builtinModules]. This will correctly mark the built-ins as external, preventing them from being bundled.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/bundler-plugin-core/rollup.config.mjs#L1
Potential issue: The `rollup.config.mjs` for `bundler-plugin-core` fails to list Node.js
built-in modules like `fs`, `os`, `path`, and `crypto` in its `external` configuration.
The package's source code uses these modules. Consequently, Rollup will attempt to
bundle these built-ins instead of treating them as external dependencies. This will lead
to runtime errors when the distributed bundle is consumed by other tools, as the
references to these Node.js modules will not be resolved correctly.
I'm hoping to get more integration tests in place before I tackle this. We don't even test Rolldown yet! |

Bonuses
This PR:
LIB_VERSIONto a file so we don't have to replace it at build time