fix: throw error if experimental.hmr is set for APIs other than dev#6860
fix: throw error if experimental.hmr is set for APIs other than dev#6860graphite-app[bot] merged 1 commit intomainfrom
experimental.hmr is set for APIs other than dev#6860Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thinking about this. I guess we should make |
Yeah. We should remove hmr option and only enable it when using |
|
@copilot the validation should be done after |
|
@hyf0 so, does the change in this PR make sense (i.e. throwing an error)? or do you think the option should be ignored and a warning should be output instead? |
Fixed in 098e141. The validation now occurs after
|
|
@copilot merge the main branch and move the validation on rust side |
Done in 62db1e7. Merged main branch and moved all HMR validation to the Rust side:
|
Benchmarks Rust |
|
@copilot move the test in |
Done in fa4b363. Moved the test from
|
crates/rolldown_binding/src/utils/create_bundler_from_binding_options.rs
Outdated
Show resolved
Hide resolved
experimental.hmr is set for APIs other than dev
hyf0
left a comment
There was a problem hiding this comment.
Nicely done. I still feel there're some bad code smells here. Let me illustrate in a separate issue without blocking this PR.
Merge activity
|
…v` (#6860) - [x] Add validation to throw an error when `experimental.hmr` is used with APIs other than `dev` - [x] Implement validation in Rust on the binding layer for `scan`, `generate`, `write` methods - [x] Implement validation in `watch` API - [x] Implement validation in `build` API - [x] Add tests for the validation - [x] Test the changes to ensure the fix works - [x] Move validation to after options hooks are called (per review feedback) - [x] Merge main branch and move validation to Rust side (per review feedback) - [x] Move HMR test from TypeScript to Rust (per review feedback) - [x] Extract HMR validation to shared function (per review feedback) - [x] Rename test directory to issue_4818 and add assertions (per review feedback) - [x] Use bundler_initialize_error instead of custom error type (per review feedback) <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[Bug]: watch + hmr crashes on change</issue_title> > <issue_description>### Reproduction link or steps > > ```js > import { watch } from 'rolldown'; > await watch({ input: 'main.js', experimental: {hmr: true }}); > ``` > > Run: > > ```bash > > echo 'console.log(1)' > main.js > > node watch.js & > > cat dist/main.js > > //#region rolldown:runtime > ... > //#endregion > //#region rolldown:hmr > ... > //#endregion > //#region main.js > var main_exports = {}; > const main_hot = __rolldown_runtime__.createModuleHotContext("main.js"); > __rolldown_runtime__.registerModule("main.js", { exports: main_exports }); > console.log("1"); > > //#endregion > > > echo 'console.log(2)' > main.js > > thread 'tokio-runtime-worker' panicked at crates/rolldown/src/stages/scan_stage.rs:73:35: > internal error: entered unreachable code: Please normalized first > note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace > node:internal/process/promises:394 > triggerUncaughtException(err, true /* fromPromise */); > ^ > > [Error: internal error: entered unreachable code: Please normalized first] { > code: 'GenericFailure' > } > > Node.js v22.17.0 > [1] + exit 1 node watch.js > ``` > > ### What is expected? > > Should not crash > > ### What is actually happening? > > I believe that after the first build the incremental-build cache is moved into the HMR manager. > On the next watcher cycle the bundler tries to perform an incremental (partial) scan, but the > `ScanStageCache` is empty, so `merge` is called without a snapshot and immediately converts > `ScanStageOutput` into `NormalizedScanStageOutput`. > That conversion expects a dense `HybridIndexVec::IndexVec`, but a partial scan always returns > `HybridIndexVec::Map`, triggering the `unreachable!("Please normalized first")` here: https://github.com/rolldown/rolldown/blob/main/crates/rolldown/src/stages/scan_stage.rs#L73 > > ### System Info > > ```Shell > System: > OS: macOS 15.5 > CPU: (8) arm64 Apple M1 Pro > Memory: 124.58 MB / 16.00 GB > Shell: 5.9 - /bin/zsh > Binaries: > Node: 22.17.0 - ~/.asdf/cache/installs/nodejs/22.17.0/bin/node > npm: 10.9.2 - ~/.asdf/cache/plugins/nodejs/shims/npm > pnpm: 10.10.0 - ~/.asdf/cache/shims/pnpm > bun: 1.1.36 - ~/.bun/bin/bun > Browsers: > Chrome: 138.0.7204.94 > Safari: 18.5 > ``` > > ### Any additional comments? > > _No response_</issue_description> > > <agent_instructions>return error if `exprimental.hmr` is set for APIs other than `dev` (e.g. `scan`, `.generate`, `.write`)</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@hyf0</author><body> > > it looks like `hmr` is not supposed to work with `watch` > > Yes. > > > and hmr needs dev server integration > > You could refer to https://github.com/rolldown/rolldown/blob/main/packages/test-dev-server/package.json. > > While we don't forbid users to implement their own hmr mechanism, but you should just use rolldown-vite to use it.</body></comment_new> > </comments> > </details> - Fixes #5217 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
de43e4d to
c878c68
Compare
…v` (#6860) - [x] Add validation to throw an error when `experimental.hmr` is used with APIs other than `dev` - [x] Implement validation in Rust on the binding layer for `scan`, `generate`, `write` methods - [x] Implement validation in `watch` API - [x] Implement validation in `build` API - [x] Add tests for the validation - [x] Test the changes to ensure the fix works - [x] Move validation to after options hooks are called (per review feedback) - [x] Merge main branch and move validation to Rust side (per review feedback) - [x] Move HMR test from TypeScript to Rust (per review feedback) - [x] Extract HMR validation to shared function (per review feedback) - [x] Rename test directory to issue_4818 and add assertions (per review feedback) - [x] Use bundler_initialize_error instead of custom error type (per review feedback) <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[Bug]: watch + hmr crashes on change</issue_title> > <issue_description>### Reproduction link or steps > > ```js > import { watch } from 'rolldown'; > await watch({ input: 'main.js', experimental: {hmr: true }}); > ``` > > Run: > > ```bash > > echo 'console.log(1)' > main.js > > node watch.js & > > cat dist/main.js > > //#region rolldown:runtime > ... > //#endregion > //#region rolldown:hmr > ... > //#endregion > //#region main.js > var main_exports = {}; > const main_hot = __rolldown_runtime__.createModuleHotContext("main.js"); > __rolldown_runtime__.registerModule("main.js", { exports: main_exports }); > console.log("1"); > > //#endregion > > > echo 'console.log(2)' > main.js > > thread 'tokio-runtime-worker' panicked at crates/rolldown/src/stages/scan_stage.rs:73:35: > internal error: entered unreachable code: Please normalized first > note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace > node:internal/process/promises:394 > triggerUncaughtException(err, true /* fromPromise */); > ^ > > [Error: internal error: entered unreachable code: Please normalized first] { > code: 'GenericFailure' > } > > Node.js v22.17.0 > [1] + exit 1 node watch.js > ``` > > ### What is expected? > > Should not crash > > ### What is actually happening? > > I believe that after the first build the incremental-build cache is moved into the HMR manager. > On the next watcher cycle the bundler tries to perform an incremental (partial) scan, but the > `ScanStageCache` is empty, so `merge` is called without a snapshot and immediately converts > `ScanStageOutput` into `NormalizedScanStageOutput`. > That conversion expects a dense `HybridIndexVec::IndexVec`, but a partial scan always returns > `HybridIndexVec::Map`, triggering the `unreachable!("Please normalized first")` here: https://github.com/rolldown/rolldown/blob/main/crates/rolldown/src/stages/scan_stage.rs#L73 > > ### System Info > > ```Shell > System: > OS: macOS 15.5 > CPU: (8) arm64 Apple M1 Pro > Memory: 124.58 MB / 16.00 GB > Shell: 5.9 - /bin/zsh > Binaries: > Node: 22.17.0 - ~/.asdf/cache/installs/nodejs/22.17.0/bin/node > npm: 10.9.2 - ~/.asdf/cache/plugins/nodejs/shims/npm > pnpm: 10.10.0 - ~/.asdf/cache/shims/pnpm > bun: 1.1.36 - ~/.bun/bin/bun > Browsers: > Chrome: 138.0.7204.94 > Safari: 18.5 > ``` > > ### Any additional comments? > > _No response_</issue_description> > > <agent_instructions>return error if `exprimental.hmr` is set for APIs other than `dev` (e.g. `scan`, `.generate`, `.write`)</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@hyf0</author><body> > > it looks like `hmr` is not supposed to work with `watch` > > Yes. > > > and hmr needs dev server integration > > You could refer to https://github.com/rolldown/rolldown/blob/main/packages/test-dev-server/package.json. > > While we don't forbid users to implement their own hmr mechanism, but you should just use rolldown-vite to use it.</body></comment_new> > </comments> > </details> - Fixes #5217 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
c878c68 to
fdb32ba
Compare
## [1.0.0-beta.50] - 2025-11-12 ### 🚀 Features - rolldown: oxc_resolver v11.13.2 (#6956) by @Boshen - rolldown: oxc v0.97.0 (#6940) by @Boshen ### 🐛 Bug Fixes - handle error when creating output chunk directories (#6953) by @sapphi-red - throw error if `experimental.hmr` is set for APIs other than `dev` (#6860) by @Copilot - apply output.paths transformation to chunk.imports and generated code (#6923) by @Copilot ### 🚜 Refactor - use `anyhow::Context::context` where applicable (#6952) by @sapphi-red - dev: use actor design pattern and allow to recover from intial build error (#6936) by @hyf0 ### 📚 Documentation - add sitemap (#6929) by @mdong1909 ### 🧪 Testing - validate publishConfig.exports matches exports field (#6950) by @Copilot - re-enable output/paths/function test (#6934) by @Copilot ### ⚙️ Miscellaneous Tasks - rename `rolldown_watcher` to `rolldown_fs_watcher` (#6958) by @hyf0 - enable trust policy (#6948) by @iiio2 - deps: update napi (#6951) by @renovate[bot] - deps: update dependency rolldown-plugin-dts to v0.17.6 (#6947) by @renovate[bot] - deps: update npm packages (#6937) by @renovate[bot] - deps: update dependency tsdown to v0.16.2 (#6933) by @renovate[bot] ### ❤️ New Contributors * @mdong1909 made their first contribution in [#6929](#6929) Co-authored-by: sapphi-red <[email protected]>
experimental.hmris used with APIs other thandevscan,generate,writemethodswatchAPIbuildAPIOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.