feat: deno run --unstable-hmr

[bartlomieju]

This commit adds --hmr flag, that enabled Hot Module Replacement.

This flag works like --watch and accepts the same arguments. If
HMR is not possible the process will be restarted instead.

Currently HMR is only supported in deno run subcommand.

Upon HMR a CustomEvent("hmr") will be dispatched that contains
information which file was changed in its details property.

[marvinhagemeister]

I tried to compile this branch, but ran into the following errors:

error[E0624]: method `receive_from_v8_session` is private
  --> cli/tools/run/hot_reload/mod.rs:89:26
   |
89  |         _ = self.session.receive_from_v8_session() => {}
   |                          ^^^^^^^^^^^^^^^^^^^^^^^ private method
   |
  ::: /Users/marvinhagemeister/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.221.0/inspector.rs:804:3
   |
804 |   async fn receive_from_v8_session(&mut self) {
   |   ------------------------------------------- private method defined here

error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> cli/tools/run/hot_reload/mod.rs:74:21
  |
74 |                 let src = tokio::fs::read_to_string(path).await?;
  |                     ^^^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
  = note: all local variables must have a statically known size
  = help: unsized locals are gated as an unstable feature

error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> cli/tools/run/hot_reload/mod.rs:74:27
  |
74 |                 let src = tokio::fs::read_to_string(path).await?;
  |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
  = note: all local variables must have a statically known size
  = help: unsized locals are gated as an unstable feature

error[E0277]: the size for values of type `str` cannot be known at compilation time
 --> cli/tools/run/hot_reload/mod.rs:74:64
  |
74 |                 let src = tokio::fs::read_to_string(path).await?;
  |                                                                ^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `str`
note: required by a bound in `std::ops::ControlFlow::Break`
 --> /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ops/control_flow.rs:93:5

Some errors have detailed explanations: E0277, E0624.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `deno` (bin "deno") due to 4 previous errors

[bartlomieju]

@marvinhagemeister the error should go away if you tried to build after pulling this branch again.

[marvinhagemeister]

@bartlomieju Awesome, got it to work with the latest changes you pushed to the branch 👍 Played around with it and it's already amazing even in its early stages! It's soo much better than just having watch mode 💯

Some notes:

• HMR seems to only work with .js files. We'd probably want to support the other extensions too .ts, .tsx, .jsx
• I really like that this only re-evaluates the current module and not the whole graph. Evaluating the whole graph breaks in scenarios where you have a setInterval running and print a value that you imported from another module. This just works in this PR 👍
• HMR doesn't work when you change the value of a top level export like export const foo = "asdf". This is totally fine and a fair tradeoff, but if hmr fails it should just print that it failed and do a full reload like it does in "normal" watch mode. Right now when it fails it just keeps running the previous code and won't accept any further updates anymore.
• Bonus: There might be a sneaky way to make top level ESM exports work by wrapping them in an iife like export const foo = (() => "asdf")() through transpilation. I've tried that locally and it accepts the update, but it doesn't re-evaluate the the parent module that imported this value. But then again even without this, the changes in the PR are already incredibly useful. This should in no way block merging this PR.
• Seems like this limitation exists for anything top-level. Changing something inside a function works.

So, in conclusion this is likely a fair tradeoff to make. From a product standpoint the only thing I'd add before shipping is:

• support .ts, .jsx, .tsx
• fall back to full reload similar to normal watch when hot update could not be applied. But still print the current error message so that the user is made aware of that.

[bartlomieju]

Thanks for trying this out!

So, in conclusion this is likely a fair tradeoff to make. From a product standpoint the only thing I'd add before shipping is:

• support .ts, .jsx, .tsx
• fall back to full reload similar to normal watch when hot update could not be applied. But still print the current error message so that the user is made aware of that.

Very reasonable and I believe we just need a few hours of works to provide all this functionality.

In terms of DX - I really don't like deno run --watch --hot-reload foo.js - I think we should go with deno run --hmr foo.js - thoughts on this? How do we describe the difference between --watch and --hmr? Maybe --watch should just do HMR by default?

[marvinhagemeister]

In terms of DX - I really don't like deno run --watch --hot-reload foo.js - I think we should go with deno run --hmr foo.js - thoughts on this? How do we describe the difference between --watch and --hmr? Maybe --watch should just do HMR by default?

I guess this depends on how confident we are in this. Long term it makes sense to combine the two. For now going with a separate --watch and --hmr/--hot flag seems like the safest option to go with. All bundlers I know of treat HMR as an opt-in thing separately from watch, so this would match already learned behavior.

My vote is for deno run --hmr foo.js or deno run --hot foo.js.

I think the difference can be described well:

• --watch: restarts the process whenever a file change is detected
• --hmr: reload the file that changed while keeping the process alive. If the HMR update cannot be applied it falls back to restarting the process

[marvinhagemeister]

Coming off a talk with @bartlomieju and we chatted a bit about an API so that user code can be notified when an HMR update is applied. A use case for that is for Fresh where we'd like to notify the browser that the server applied an HMR update so that the browser can then initiate a new render with the updated content.

There is a bit of prior art on API designs for that in the vite ecosystem which went with a more modern spin on webpack's older HMR API. They went with import.meta.hot.accept() which is only available in HMR mode. Full docs here https://vitejs.dev/guide/api-hmr.html

For Deno though we both weren't sure if that's the right choice. VIte's API gets the module record passed as an argument which we don't have with the v8 API that we're basing this on. Instead a much more "web-like" alternative seems to instead go with a normal event dispatch instead. Something like:

addEventListener("deno:hot", ev => {
  console.log(`[hmr] Update applied to ${ev.data.file}`)
  // prints: [hmr] Update applied to file://foo/bar/baz.tsx
});

A simple event listener would be quite a powerful API as we could hook it into lots of other things too like the test runner for example. For Fresh this would be perfect as well as this allows me to ping the browser and handle the update there.

[marvinhagemeister]

Found a scenario where the process isn't restarted anymore after an error anymore:

// foo.tsx
// Uncomment these lines and comment them again
// import foobar from "bar";
// console.log(foobar);

export function foo() {
  return `<h1>a</h1>`;
}

// main.ts
import { foo } from "./foo.tsx";

let i = 0;
setInterval(() => {
  console.log(i++, foo());
}, 1000);

Steps to reproduce:

1. Uncomment the invalid import in foo.tsx -> Error is correctly shown
2. Comment the invalid import again -> server is stuck

[marvinhagemeister]

Came across another issue:

With --watch you can pass unrelated files to watch upon which an event is triggered. With --hmr these are not detected if they are not included in the module graph.

[bartlomieju]

Thanks for reporting that Marvin, both of these problems have been handled and are restarting properly 👍

[dsherret]

This will be a nice feature.

[dsherret]

LGTM!