webpack-contrib/mini-css-extract-plugin

HMR implementation is unnecessary intrusive into `<link>` nodes?

birdofpreyru opened this issue · 3 comments

birdofpreyru commented

Modification Proposal

Expected Behavior / Situation

My expectation for HMR implementation is to be as little intrusive into DOM as possible. I believe, to reload CSS chunk it is enough to just alter <link>'s HREF by updating some query param (as it is done now), but it is not needed to remove existing <link> node and replace it by its clone.

Actual Behavior / Situation

The current implementation of updateCss() clones and replaces existing <link> nodes. I am not sure why is it necessary (and I hope you could clarify it to me), but it definitely causes a few shortcomings:

  • el.cloneNode() does not necessarily produces an exact clone of the original node. It loses custom fields attached to it, e.g. if the host code does some assignments to that node like el.fields = someValue, or el.dataset.key = someValue < these data are currently lost in the result of HMR.
  • Current HMR implementation normalizes HREF format, e.g. if the original <link> URL was /an/absolute/path, the HMR unnecessarity turns it into schema://domain/an/absolute/path?timestamp (I believe, just adding/modifying the timestamp would be enough to reload the chunk).
  • As the current implementation does newEl.href = `${url}?${Date.now()}`; it looses any custom query parameters of the <link> HREF. IMHO, if the original HREF had a query string, the HMR implementation should correctly parse and modify it, rather than throwing the original query away.

Because of these points, the current HMR implementation is likely to unnecessary interfere with the host code operating <link> nodes in some custom manner, e.g. for operating code-splitting, etc..

Please paste the results of npx webpack-cli info here, and mention other relevant information

alexander-akait commented

Yes, we can improve it

el.cloneNode() does not necessarily produces an exact clone of the original node. It loses custom fields attached to it, e.g. if the host code does some assignments to that node like el.fields = someValue, or el.dataset.key = someValue < these data are currently lost in the result of HMR.

Because we don't have chunks here, it is just module, and we assume that each such module will be a chunk, we have #708 where this logic does not work

Current HMR implementation normalizes HREF format, e.g. if the original URL was /an/absolute/path, the HMR unnecessarity turns it into schema://domain/an/absolute/path?timestamp (I believe, just adding/modifying the timestamp would be enough to reload the chunk).

And yes and no, some developers uses custom plugins to generated HTML, and it can be problem, we tried to provide better DX, but unfortunately most of the time it still doesn't work, anyway we can avoid it if you have absolute URL starting with /

As the current implementation does newEl.href = ${url}?${Date.now()}; it looses any custom query parameters of the HREF. IMHO, if the original HREF had a query string, the HMR implementation should correctly parse and modify it, rather than throwing the original query away.

It should be easy to fix so PR welcome

  • 👍1
birdofpreyru commented

Sorry, I am not following you on the first two points. Are chunks / modules / higher-level concepts relevant at all to what updateCss() does? Isn't it just takes a <link> element existing in the DOM and forces the browser to reload it, and wouldn't it work exactly the same if you just mutate the URL in any way (assuming the server still returns the same file for the new URL), or you remove then re-insert the node into DOM?

alexander-akait commented

Isn't it just takes a element existing in the DOM and forces the browser to reload it, and wouldn't it work exactly the same if you just mutate the URL in any way (assuming the server still returns the same file for the new URL), or you remove then re-insert the node into DOM?

The problem is that the chunk creation happens after building modules, we have plan to implement built-in CSS support in webpack core and it allows to fix this problem.

We are cloning https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/hmr/hotModuleReplacement.js#L106, there is other potential problem, you can have listeners, I don't use this usage in real works, but it could be.

You can look at code https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/hmr/hotModuleReplacement.js#L106 and send fixes, there are actually some things are wrong and can be improved, keeping queries is not a problem and yes we can avoid cloning