Report compile error to the client

[rixo]

Aaaah, I'm too excited about this!

I've look at how Webpack Dev Server does it: https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L178. They use a compiler hook and write errors on the WS as 'error' event. Apparently, the WS client then publishes this with a custom event on window, for consumption by "plugins". In those, I bet there is the one implementing the module.hot protocol. On the other hand, the WS client does process the compile error & implement the overlay itself.

I'm not a fan of how this is hard to track...

So! I couldn't resist to see how my Svelte HMR would look with compile error reporting, so I spun a test implementation around your idea of module.hot.compile: master...rixo:compile-error-reporting

And here's the code using it: https://github.com/rixo/rollup-plugin-svelte-hmr/blob/master/index.js#L77 (and the demo, if you're into Svelty things -- I hooked it up to my fork for now).

(It looks very well! That's just some very very preliminary very early impression but the whole thing seems flawless... On the paper, it might be better than React Fast Refresh. LOL. Svelte's awesome.)

Anyway, to us... As you can see, I didn't do module.hot.compile but window.__hot.addErrorHandler. That's because I realized that when we get a compile error, it can't be targeted at one module, the whole bundle has to be dead, right?

Were you thinking of propagating the bundle error to all module instances (i.e. call every module.compile callback)?

If so, this way would be very impractical to manage something like an error overlay. We only want to control it from a central location that does not get affected by code reload. We want it central because it has to manage singleton assets, e.g. its overlay's DOM element. Like the WS itself needs to be central to manage its connection.

The questions I've asked myself when doing this:

• Starting from the WS message that is written by the server: should it contains errors or only allow for one single error? Is it possible to somehow have multiple compile errors simultaneously?

• Should the server (or the WS client) emit null when the compile error is cleared (i.e. on successful compile)? That makes consumer implementation more straightforward, at least.

Also, if we agree that the addErrorHandler API should be central rather than on module, how to expose it to consumer? I guess window.__hot is supposed to be private? Maybe pass it to nollupBundleInit? That's more restricting but it might be cleaner. That would make nollupBundleInit callbacks the only possible place you have to search to find HMR/WS client related code in plugins.

Ah! And just to mention it, what would be the final touch for error reporting would be that the server manages to deliver something anyway when it starts with an initial compile error. Maybe send a blank script with just the HMR clients (nollupBundleInit). This way we could display the initial error in the browser, like WDS (it does a full reload when the error is fixed). Don't know how doable that would be. But that would really be the icing on the cherry anyway.

So, what do you think?

[PepsRyuu]

Apologies for the delay in responding. Been a busy week! :)

Just for clarity and for ensuring there's as much information as possible, this seems to be how Webpack does things:

• As mentioned, the websocket publishes an errors event which gets sent over to the client runtime. There's also an ok event published whenever the build is successful.

• For Webpack, they provide an --overlay option, with the ability to individually toggle warning and error overlays. This overlay is implemented by the HMR client that Webpack provides. https://github.com/webpack/webpack-dev-server/blob/master/client-src/default/overlay.js

• Third party plugins create their own websocket connection to the HMR engine, listening for the same events. https://github.com/G-Rath/webpack-serve-overlay/blob/master/errorOverlay.js This websocket information is passed into the plugins through the compiler instance with compiler.options.devServer.sockHost and others: https://github.com/smooth-code/error-overlay-webpack-plugin/blob/master/src/index.js

I suppose the question here is whether or not to follow a similar convention to what Webpack is using. Here's a couple of thoughts:

• Must use nollupBundleInit to inject custom code. I don't think there's any problem with this at all. It's pretty much designed for the purpose of injecting once off development code.

• Overlays to be implemented by third party plugin, no first party overlay. I have a feeling that people will want to have different types of overlays. Rather than having a fullscreen overlay, perhaps someone might want an overlay that's more minimalistic that doesn't block the view of the app. Instead, Nollup should provide some generic capability to enable this feature.

• Having separate events for ok and errors seems like a good idea rather than using null. Having an explicit ok makes it less ambiguous in my opinion in regards to the condition of the bundle. As for errors, it would have to be a single error as that's what would be captured by the catch error handler, not entirely sure how multiple errors would work.

• I do think the best solution is to have a clean API rather than having plugins creating new websocket connections. window.__hot is indeed supposed to be private, but there could be something new added for this purpose. I agree that the idea of module.hot.compile might not be the way to go because it would be on each module as opposed to a global basis.

• Perhaps something like: compiler.addStatusHandler('ok') and compiler.addStatusHandler('errors') might be the way to go. Not tying it to HMR at all because it not related at all to it, and should be allowed to be used independently. addStatusHandler would keep it in line with the HMR runtime's method as well. An issue with this though is that there might need to be a separate websocket independent of HMR for the purpose of showing compiler events. Just throwing ideas out there.

[PepsRyuu]

Here's Parcel's implementation for reference as well:

https://github.com/parcel-bundler/parcel/pull/1074/files

https://github.com/parcel-bundler/parcel/blob/master/packages/core/parcel-bundler/src/HMRServer.js

It's tied to the HMR websocket connection, and uses error and error-resolved events, but it looks to be completely internal with a hardcoded overlay.

[rixo]

Apologies for the delay in responding. Been a busy week! :)

Ah ah, you lucky I'm not paying you!

Overlays to be implemented by third party plugin, no first party overlay.

Yes, absolutely. The overlay is a platform (browser) specific concern, while the bundler is not. Nollup could conceivably be used to compile Node code. I've also got the case of SvelteNative where a normal DOM overlay wouldn't work (even though SvelteNative can't use Nollup unfortunately -- NativeScript itself has strong ties to Webpack).

Having separate events for ok and errors seems like a good idea rather than using null.

Perhaps something like: compiler.addStatusHandler('ok') and compiler.addStatusHandler('errors') might be the way to go.

Yes, I think there should probably be 'ok' and 'error' status events. I also think that status events should remain text only to keep their responsibility focused on diagnostic (verbose log) / signal.

Now... I think I've overlooked another class of errors.

We do have compile errors: there can be only one at a time, we can know exactly when the error has been cleared, and they are always recoverable.

But I think we also need to account for runtime errors that can happen in user provided functions, that is the callbacks / listeners passed to module.hot.xxx (this for example). Most crucially, they include accept callbacks, meaning runtime errors can originate in user code under development (said otherwise: they will happen).

For those, there can be multiple at once, we can't know when/if the error(s) is cleared, and they leave the application in an undetermined state so most often they will be fatal.

In most cases, they will be user errors, and the user will expect them in the console (that's what would happen without hot reload). But I think they should still be caught by Nollup, otherwise they might invisibly break HMR state (remaining callbacks not getting called).

For a third party HMR plugin, runtime errors handling would probably be : reporting (overlay) + full reload on next update (while for compile errors it's : show overlay + hide overlay). Well, that would be for the plugin to decide, but Nollup error API should make it possible / easy to distinguish between compile & runtime errors.

So, maybe something like this?

// a flag to distinguish between error types?
addErrorHandler(errors) {
  needFullReload = errors.some(error => !error.isCompileError)
  // ...
}

I'm not a fan of this loop though. Nor tampering with the error objects. I think I'd prefer something more explicit:

// compile is one error or falsy
// runtime is an array of errors or falsy
// errors is an array with compile + runtime errors, for the cases 
// where the distinction is not relevant
addErrorHandler({ errors, compile, runtime }) {
  needFullReload = !!runtime
  // ...
}

// or maybe
addErrorHandler(errors, { compile, runtime }) { ... }

Regarding transport, what would you think of something like the following?

nollupBundleInit ({ hot }) {
  return `
    ${hot}.addErrorhandler( ... )
    ${hot}.addStatusHandler( ... )
  `
}