FredKSchott / esm-hmr

a Hot Module Replacement (HMR) API for your ESM-based dev server.
MIT License
413 stars 11 forks source link

Feature: Error Reporting #3

Open FredKSchott opened 4 years ago

FredKSchott commented 4 years ago

We should support an "error" event type so that the server can notify the client whenever an error occurred at the build stage.

Current Behavior

Expected Behavior

rixo commented 4 years ago

I think error reporting is the heart of good HMR experience. Fully reliable HMR remains a pipe dream in practice, and error management is often left behind on the base that "we're just a dev luxury feature, so losing track of state in edge cases is acceptable". I personally disagree with this stance, because proper error management and reporting is precisely what can make the difference between the user understanding what is going on and things seemingly randomly working or not -- said otherwise, between HMR being perceived as predictible and reliable vs random dark magic. Empowering users also increase the probability of having problems reported correctly and, even, reported at all.

For this reason, I think accept / dispose callbacks should be kept async (reacting to a previous comment for @FredKSchott in another issue I can't find right now).

I'm hijacking this issue with this because async callbacks are meant for better error management.

The use case is that some lib specific adapters can have async dispose / accept logic. For example, in Svelte, we can have outros animations running when a component is removed (to be replaced by a new version). This is obviously async. Now, if something wrong happens when we create the new version of the component, this should be considered a HMR error, an error that can only ever happens because we're attempting HMR. Typically (depending on user's taste), this would warrant a full reload.

These errors that happen in HMR specific code (i.e. accept handlers) should be distinguished from errors that happen in user's code. They can both happen following the same trigger, that is a HMR update has happened, but they're not the same in nature. An error that happens when you run the new module's code is a user error. By contrast, one that happens in an accept handler is a HMR specific error.

IMO user errors don't warrant a full reload / HMR giving up. I mean, you wouldn't reload the browser on any random runtime error during the app's lifecycle (eg "foo is not a function") just because your dev server has HMR capabilities. Likewise, I believe user errors should be left visible in the browser, even if they are caused by a HMR failed attempt at preserving some piece of state between updates (oftentimes, such HMR crashes only make apparent existing holes in the app's normal resource lifecycle / disposal management).

Flawless HMR implies perfect failure reporting. This needs to be able to differentiate between userland errors and HMR specific errors. And you won't be able to tell between all of them if your HMR handlers are synchronous, because a HMR adapter can realize it's in an inextricable situation only asynchronously (e.g. after outros have run, and the HMR adapter finds itself with a runtime exception after trying to instantiate the new component). It won't have the opportunity to escalate the error meaningfully, if the HMR engine thinks its update has already been fully applied at this point.

In a nutshell, I believe good error reporting is crucial to good HMR, and async HMR callbacks are a requirement for this. They're next gen HMR stuff.

FredKSchott commented 4 years ago

That's right, I remember making this change as well but also couldn't find the comment.

I think this was moved to a sync API due to event bubbling, and the concern around processing different events in parallel (this ones being applied, this ones being disposed, etc.) All other implementations that I've found seem to be synchronous as well, although I'd love some references to specs/implementations that do this well, if you have any.

rixo commented 4 years ago

No, I don't know of other implementations being asynchronous. This is forward thinking.

That being said, if a HMR adapter needs to be async (because its target framework requires an async processing on dispose / init), it will be async, whether the underlying HMR client supports it or not. If the HMR client don't support it, it will just miss the capacity to really know when a HMR update is really completely applied (for example, it would console.log "everything up to date" before the HMR adapter has actually updated the screen -- and possibly will crash).

My own HMR implementation (rollup-plugin-hot) is async, so at least it proves that it is possible.

I did have async issues when I first tried it with Snowpack, before the switch to Chokidar, because I was getting duplicated change events for the same files. I was able to fix it by making sure the previous batch of events was completely processed before allowing a new one to begin.

FredKSchott commented 4 years ago

That's a great point, that an async adapter can still hook into a sync adapter, you just lose the error reporting and any guarentees that cleanup work happens before new update. Honestly, performance cost of having to wait for async dispose calls makes me think this is a worthwhile tradeoff, at least for now.

Also to get back to the main issue, the feature described in OP is reporting server/build errors to the client.

rixo commented 4 years ago

Yup, sorry for derailing the thread. I do also think that reporting build error to the client is an important feature.