Prune loadable stats file more aggressively

jdb8 commented 3 years ago

🚀 Feature Proposal

This is a combination of a question and a feature proposal really: it's possible that this may not be possible.

Basically, while it looks like the webpack plugin already turns off a few unnecessary stats keys here https://github.com/gregberge/loadable-components/blob/3676e488a714068baca5cefad2aa98c0de0a8fab/packages/webpack-plugin/src/index.js#L21 my proposal is to set up an allowlist of stats rather than excluding specific ones, and the proposal would be to only include assets, chunks, and namedChunkGroups since these appear to be the only properties accessed on this.stats inside ChunkExtractor.

I'll need to look into which options webpack lets us control when outputting stats, but iirc it's basically everything. Worst case it'd still be possible to perform an additional pass and strip out anything that loadable won't be relying on.

Motivation

Even with the code setting stats options above, in my project I'm still seeing loadable-stats.json become very large in certain circumstances which takes up space on disk but more importantly slows down my require() in ssr when passing the data into ChunkExtractor.

From looking at the code in ChunkExtractor (https://github.com/gregberge/loadable-components/blob/3676e488a714068baca5cefad2aa98c0de0a8fab/packages/webpack-plugin/src/index.js), it seems like most of the properties taking up space are things like warnings, children, and other things Loadable doesn't seem to actually care about.

Example

It would be the default way of generating stats, but if there's a usecase for errors/warnings etc. then perhaps a filter function could be added as an api option: it would be a function taking the output loadable stats json object, and returning a new object.

This might also be generally useful for anyone wishing to modify their loadable stats file before it's output, for whatever reason.

Pitch

This seems overall more efficient and seems like it has little downside unless these properties are used in a way I haven't noticed.

open-collective-bot[bot] commented 3 years ago

Hey @jdb8 :wave:, Thank you for opening an issue. We'll get back to you as soon as we can. Please, consider supporting us on Open Collective. We give a special attention to issues opened by backers. If you use Loadable at work, you can also ask your company to sponsor us :heart:.

jdb8 commented 3 years ago

Looking at this again, I'm actually confused how things are working: loadable seems to rely on assets, chunks, and namedChunkGroups as mentioned, but only assets is set to true when hookCompiler.getStats().toJson() is called here in the version I'm running on disk (5.14.0): https://github.com/gregberge/loadable-components/blob/main/packages/webpack-plugin/src/index.js#L24

Although it looks like chunks was toggled from false to true in https://github.com/gregberge/loadable-components/pull/689 and released in 5.14.2, but that still leaves namedChunkGroups (set via chunkGroups: https://v4.webpack.js.org/configuration/stats/#statschunkgroups). I assume right now we're relying on default stats behaviour, since all: false is not set (which acts as a default).

So I think my proposal would be to update the call to be:

const stats = hookCompiler.getStats().toJson({
    // by default, include nothing
    all: false,
    // set the stats properties loadable needs explicitly
    hash: true,
    publicPath: true,
    outputPath: true,
    assets: true,
    chunks: true,
    chunkGroups: true,
});

This way, if new stats properties get added in future versions of webpack, they're not included unnecessarily. It also makes it clearer which items are actually used by loadable during SSR.

theKashey commented 3 years ago

There is a fresh PR with exactly this change - https://github.com/gregberge/loadable-components/pull/708

theKashey commented 3 years ago

Long story short:

Prune loadable stats file... should be disabled by default, as some (many) people are using stat files generated by loadable as 🤯 stat file :)
But ideally, the output should be a little abstracted from the webpack implementation and just save something lodable needs, and that is not much. I've tried it at https://github.com/theKashey/webpack-imported, which is bacially moves some logic from loadable/server to loadable/plugin. Works well.

jdb8 commented 3 years ago

Oh great! I should have checked open PRs before making this issue.

Prune loadable stats file... should be disabled by default, as some (many) people are using stat files generated by loadable as 🤯 stat file :)

The linked PR isn't doing that though, right? Is the plan to gate that change of behaviour behind a new flag? The fact that loadable already omits some data that would otherwise be present in stats.json means that anyone relying on loadable-stats.json to be equivalent is likely already experiencing bugs 😬

But ideally, the output should be a little abstracted from the webpack implementation and just save something lodable needs, and that is not much.

Agreed - it would also mean that you're not tied to the representation that webpack uses, which is more flexible for future changes.

~I see that in the linked PR you still keep some items explicitly set to false despite setting all: false - is that necessary? Seems identical to what I had in mind otherwise :)~

EDIT: just realised this isn't your PR, sorry! I'll comment on there instead.

stale[bot] commented 3 years ago

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.

jdb8 commented 3 years ago

I believe this is now fixed as of https://github.com/gregberge/loadable-components/pull/708 and https://github.com/gregberge/loadable-components/pull/735, right @theKashey?

stale[bot] commented 3 years ago

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.

theKashey commented 3 years ago

🐌 @jdb8 - seems so. However now we are going to extend information a little bit, as well as there is an idea to allow controlling stats generation from the client-side, to make it a little bit reusable.

stale[bot] commented 3 years ago

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.

gregberge / loadable-components