nollup: Shouldn't this line use `every`?

This line, shouldn’t it be every instead of some? We need all impacted branches to independently accept the change, don’t we?

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 27 (27 by maintainers)

Most upvoted comments

Nice! I think I’ll take inspiration from this test approach some day…

I hear you about Webpack hot API. Took me forever to begin to make sense of it.

So! I went ahead and tested the above cases in Parcel. (In fact, I ended up creating a repository to test the behaviour of each bundler. It implements my 2nd scenario which, I think, is the most tricky. I’ve added a summary of the results in the README.)

To my great surprise Parcel’s results do come different from Webpack.

In the first case, the HMR update is accepted even though the C branch has no accept handler.

In the second, the update is accepted BUT only one of the two possible accept handlers is run. Which one is run depends on the order in which the modules (with accept handlers) are imported.

In the last scenario, no surprise, the update is accepted and the handler is run only once.

OK so I guess this matches Nollup’s current behaviour?

I must say I remain very puzzled by this behaviour. Webpack makes more sense to me on this point. It evens seems at odd with Parcel’s docs itself:

You call module.hot.accept with a callback function which is executed when that module or any of its dependencies are updated.

This is not true in my second scenario, since C has a accept handler that is not called although it has A as a dependency and A has been updated.

I’m inclined to think this should be considered a bug in Parcel.

My theory is that the “bug” might go essentially unnoticed in practice because most HMR adapters (e.g. React, Vue…) target Webpack as their first implementation and so they don’t rely on accept handlers being called. So this difference in behaviour between Webpack and Parcel / Nollup does not matter. Furthermore, HMR is mostly used to replace UI components (e.g. Svelte, React. Vue…) and, as far as I can tell, these adapters all put an accept handler in every relevant (i.e. component) module. So the difference in regard of rerunning consumer (parent) modules also don’t matter for this use case.

For app specific usages of the hot API though (e.g. clearInterval…), I think Parcel’s behaviour probably causes subtle HMR bugs because it fails to rerun the modules in some of the branches that are affected by a HMR update.

In the example, I consider that module C is broken after the HMR update with Parcel, because it has not been rerun with updated bindings for A. Using this module after the update will produce outdated effects. Further, even though I think it’s correct to not rerun the main module because the bubble has been accepted before it in all branches, I still consider that main is broken after the update with Webpack, Parcel, and Nollup, because it uses outdated bindings (Webpack docs state that bindings are updated automatically when using ES import).

Do you you share these conclusions? Or is there something I have overlooked?

Also, my test repo shows that Nollup fails to call dispose handlers for some of the modules it does rerun (module B in the example). I’ve not investigated but that should probably be an easy fix. Especially with your new tests 😉

And, since we’ve got our hands in this… While I was experimenting with Parcel, I saw that Nollup’s API was mirroring it almost exactly, with the one notable difference that Nollup does not support module.hot.data / module.hot.dispose(data => { ... }). This is supported with the same API in Webpack. That’s a feature I like because it prevents having to mess with outer scope (e.g. window) to pass information between two versions of a module, and it’s cleanly cut for this job. Do you think this should be added to Nollup?