Skip to content

Comments

fix: throw error if experimental.hmr is set for APIs other than dev#6860

Merged
graphite-app[bot] merged 1 commit intomainfrom
copilot/fix-watch-hmr-crash
Nov 11, 2025
Merged

fix: throw error if experimental.hmr is set for APIs other than dev#6860
graphite-app[bot] merged 1 commit intomainfrom
copilot/fix-watch-hmr-crash

Conversation

Copy link
Contributor

Copilot AI commented Nov 5, 2025

  • Add validation to throw an error when experimental.hmr is used with APIs other than dev
  • Implement validation in Rust on the binding layer for scan, generate, write methods
  • Implement validation in watch API
  • Implement validation in build API
  • Add tests for the validation
  • Test the changes to ensure the fix works
  • Move validation to after options hooks are called (per review feedback)
  • Merge main branch and move validation to Rust side (per review feedback)
  • Move HMR test from TypeScript to Rust (per review feedback)
  • Extract HMR validation to shared function (per review feedback)
  • Rename test directory to issue_4818 and add assertions (per review feedback)
  • Use bundler_initialize_error instead of custom error type (per review feedback)
Original prompt

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

import { watch } from 'rolldown';
await watch({ input: 'main.js', experimental: {hmr: true }});

Run:

> 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

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)

@hyf0 > 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.</comment_new>


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 5, 2025

How to use the Graphite Merge Queue

Add 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.

@netlify
Copy link

netlify bot commented Nov 5, 2025

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit fdb32ba
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/69131c930a978d000807f91e
😎 Deploy Preview https://deploy-preview-6860--rolldown-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@hyf0
Copy link
Member

hyf0 commented Nov 5, 2025

Thinking about this. I guess we should make hmr conflict with using watch mode.

@hyf0
Copy link
Member

hyf0 commented Nov 5, 2025

Thinking about this. I guess we should make hmr conflict with using watch mode.

Yeah. We should remove hmr option and only enable it when using dev engine internally to avoid errors.

Copilot AI changed the title [WIP] Fix crash when using watch with HMR fix: prevent HMR usage with non-dev APIs to avoid crash Nov 5, 2025
Copilot AI requested a review from sapphi-red November 5, 2025 12:19
@sapphi-red
Copy link
Member

@copilot the validation should be done after options hooks are called as options hooks may update the option

@sapphi-red
Copy link
Member

@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?

Copy link
Contributor Author

Copilot AI commented Nov 6, 2025

@copilot the validation should be done after options hooks are called as options hooks may update the option

Fixed in 098e141. The validation now occurs after options hooks are called:

  • In build(), validation moved to after rolldown() (which calls options hook)
  • In watch(), validation moved to after callOptionsHook()
  • Added tests to verify plugins can modify experimental.hmr in their options hook before validation

@sapphi-red
Copy link
Member

@copilot merge the main branch and move the validation on rust side

Copy link
Contributor Author

Copilot AI commented Nov 10, 2025

@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:

  • Removed TypeScript validation from validator.ts, build.ts, and watcher.ts
  • Added HmrNotAllowedForApiError error type in Rust
  • Implemented validation in binding_bundler.rs for generate/write/scan methods
  • Implemented validation in create_bundler_from_binding_options.rs for watch API
  • All tests pass and linting is clean

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Benchmarks Rust

group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.00     68.3±3.21ms        ? ?/sec    1.02     69.8±3.63ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.01     74.9±3.84ms        ? ?/sec    1.00     74.4±2.56ms        ? ?/sec
bundle/bundle@rome_ts                                        1.00    110.4±2.75ms        ? ?/sec    1.02    112.5±2.69ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.02    127.3±3.24ms        ? ?/sec    1.00    124.4±3.43ms        ? ?/sec
bundle/bundle@threejs                                        1.00     39.1±1.49ms        ? ?/sec    1.05     41.2±1.16ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.00     42.7±0.54ms        ? ?/sec    1.06     45.1±1.12ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    404.8±9.46ms        ? ?/sec    1.00    403.1±6.69ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    466.8±9.32ms        ? ?/sec    1.00    467.3±5.27ms        ? ?/sec
scan/scan@rome_ts                                            1.01     88.1±1.99ms        ? ?/sec    1.00     87.4±1.82ms        ? ?/sec
scan/scan@threejs                                            1.02     29.2±0.66ms        ? ?/sec    1.00     28.6±0.40ms        ? ?/sec
scan/scan@threejs10x                                         1.01    304.3±6.40ms        ? ?/sec    1.00    301.3±6.23ms        ? ?/sec

@sapphi-red
Copy link
Member

@copilot move the test in packages/rolldown/tests/fixtures/misc/issues/4818/_config.ts to crates/rolldown/tests/rolldown/topics/hmr

Copy link
Contributor Author

Copilot AI commented Nov 11, 2025

@copilot move the test in packages/rolldown/tests/fixtures/misc/issues/4818/_config.ts to crates/rolldown/tests/rolldown/topics/hmr

Done in fa4b363. Moved the test from packages/rolldown/tests/fixtures/misc/issues/4818/ to crates/rolldown/tests/rolldown/topics/hmr/issue_5217/:

  • Converted TypeScript config to JSON format
  • Removed assert.mjs (Rust tests use snapshots instead)
  • Generated snapshot automatically
  • Test passes successfully

@sapphi-red sapphi-red requested a review from hyf0 November 11, 2025 05:26
@sapphi-red sapphi-red changed the title fix: prevent HMR usage with non-dev APIs to avoid crash fix: throw error if experimental.hmr is set for APIs other than dev Nov 11, 2025
@sapphi-red sapphi-red marked this pull request as ready for review November 11, 2025 05:27
Copy link
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done. I still feel there're some bad code smells here. Let me illustrate in a separate issue without blocking this PR.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 11, 2025

Merge activity

  • Nov 11, 6:13 AM UTC: sapphi-red added this pull request to the Graphite merge queue.
  • Nov 11, 6:18 AM UTC: The Graphite merge queue couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'node-test (ubuntu-latest) / Node Test', 'node-test (macos-latest) / Node Test', 'node-test (windows-latest) / Node Test').
  • Nov 11, 11:22 AM UTC: sapphi-red added this pull request to the Graphite merge queue.
  • Nov 11, 11:36 AM UTC: Merged by the Graphite merge queue.

graphite-app bot pushed a commit that referenced this pull request Nov 11, 2025
…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).
@graphite-app graphite-app bot force-pushed the copilot/fix-watch-hmr-crash branch from de43e4d to c878c68 Compare November 11, 2025 06:13
…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).
@graphite-app graphite-app bot force-pushed the copilot/fix-watch-hmr-crash branch from c878c68 to fdb32ba Compare November 11, 2025 11:22
@graphite-app graphite-app bot merged commit fdb32ba into main Nov 11, 2025
28 checks passed
@graphite-app graphite-app bot deleted the copilot/fix-watch-hmr-crash branch November 11, 2025 11:36
shulaoda pushed a commit that referenced this pull request Nov 12, 2025
## [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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: watch + hmr crashes on change

3 participants