feat: filter chunk assets

[cahnory]

Summary

In development, when doing SSR with HMR you don't want the hmr related files to be linked by the served html. The filter option in addition to the new info property added to chunk assets make it possible.

I favored this approach rather than an "excludeHmrFiles" type option as it could serve other purposes and remain relevant even if hmr is abandoned in the future.

Test plan

Using webpack with hmr enabled, instanciate your ChunkExtractor like :

import { ChunkExtractor } from '@loadable/server'

const extractor = new ChunkExtractor({
  statsFile: '../dist/loadable-stats.json',
  filter: ({ info }) => !info.hotModuleReplacement,
});

All files relative to hmr (suffixed with "-wps-hmr" by default) should be excluded from the results of getScriptTags, getScriptElements,…

[theKashey]

Fixes #652 ?

[theKashey]

Let's use a less generic name on this scope. For example filterChunk

[cahnory]

You're right, I made the change.

[cahnory]

I also renamed the option as it makes its purpose more explicit.

[cahnory]

Fixes #652 ?

I don't think so.

I didn't want to mess with loadable-stats.json as my intent was only to prevent serving hmr chunks but the stats could potentially serve other purposes.
So this PR allows ChunkExtractor to apply a filter on the assets it returns via its getter methods while #652 would requires the webpack plugin to apply a filter on the chunks it add to the loadable-stats.json file.

[cahnory]

Don't know why the ci failed on the last commit.

[cahnory]

@theKashey if you prefer that the filter occurs in the webpack plugin (exclude chunks from stats file) let me know.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cahnory]

@theKashey I'm having a problem with stats.json.

When generated, it does not contain hmr chunks (obviously) and the assets.*.info property is always empty (but maybe the hotModuleReplacement property is only set when true).
I can manipulate stats object in my test to match my needs but I think it's not the way I'm supposed to do this.

[theKashey]

Yeah, probably you should take the original stats (as it's now generated from a real project) and add some "extra" fields in runtime to test, or filter some "normal" chunks, as it does not have test hot case, only the filtering

[cahnory]

or filter some "normal" chunks, as it does not have test hot case, only the filtering

I had my initial usecase so much on my mind that I didn't think about it!

[theKashey]

👀 .....

[cahnory]

😅 I don't know what's wrong. I run the commands ci, test and even build locally without any problem. It seems to me that it has something to do with a deployment step that I don't master. In netlify logs I just see :

failed Building production JavaScript and CSS bundles

Any idea ?

[theKashey]

@cahnory - it's just dead a little. Strangely there is no travis here, and these tests are the only really important thing.

[eliseumds]

Anyway I can help here?

[theKashey]

The real problem here is a "new" undocumented feature, which implementation is still on the user.
If HMR temporal files should not be a part of webpack builds - then they should not. Full stop and no extra conditions.

[cahnory]

The real problem here is a "new" undocumented feature, which implementation is still on the user.
If HMR temporal files should not be a part of webpack builds - then they should not. Full stop and no extra conditions.

This is not what I understood from our previous exchanges. In this case, if I understand correctly, it seems unsolvable and maybe we should close this PR.

[theKashey]

This is not what I understood from our previous exchanges.

Decisions change in time. That's ok.

if I understand correctly, it seems unsolvable

The internal changes are legit and I would like to have them. I am just thinking that we should, at least for now, put filter: ({ info }) => !info.hotModuleReplacement, inside loadable logic, keep external API intact as and I am not sure is there any other use case for filter. If you have one - feel free to change my mind.

The lazy maintainer rule is simple - if you can not expose something - do not, or you will have to maintain it forever (and you are lazy by definition).

[cahnory]

I understand, no problem. Just that by doing this way this PR should be closed in favor of a new one which maybe just add a cookbook recipe to the doc.

The way I do it right now is just by filtering extractor.getXElements results with this function:

const filterHmr = (list) =>
  list.filter(
    (element) =>
      !element.props.src?.match(/wps-hmr\.[^.\\/]+$/) &&
      !element.props.href?.match(/wps-hmr\.[^.\\/]+$/),
  );

I just wasn't satisfied with having to check the file names rather than their stats (and extra loops but they are negligible).

[theKashey]

That's why it's still good to have the majority of this PR built-in into loadable with the correct behavior working out of the box

[cahnory]

That's why it's still good to have the majority of this PR built-in into loadable with the correct behavior working out of the box

Oh I see, I completely misunderstood what you were saying. Sorry.
I'll try to find a moment to make the changes.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.