CI: Report tree-shaking size in PR

[marcofugaro]

Related issue: #25470 (comment)

Description
If a PR edits something inside the src/ folder, this comment will appear on the PR:

The comment will later be updated after changes in the PR.

/cc @LeviPesin @epreston

[LeviPesin]

Is it the full build size or just the test tree-shaken build size? I think it would be great to report both.

And maybe we can report brotlied size instead or as addition to the gzipped size? I think brotli is starting to be used in various places so maybe it can reflect better the "true" size that user should download on network.

[marcofugaro]

Is it the full build size or just the test tree-shaken build size? I think it would be great to report both.

Yeah makes sense, I personally don't want to know the full library size but I see how it could be useful for some people.

And maybe we can report brotlied size instead or as addition to the gzipped size? I think brotli is starting to be used in various places so maybe it can reflect better the "true" size that user should download on network.

Ah I just copied what vite/create-react-app do, let's leave gzip since it's the most common one. It's on the user to decide if he wants to use brotli compression and reduce the bundle even more.

[epreston]

This is awesome. Let's update the test-treeshake to be as accurate as possible to take advantage of this.

[epreston]

Brotli will be smaller. GZIP beats Brotli on speed. It's up to the web server config and negotiation with each users browser if it's even used. Developers don't have a choice. I hope people remember to optimise for accuracy and size, not how compressible text might be.

[epreston]

I'll get started on the test-treeshake update if that's not taken already.

[marcofugaro]

I'll get started on the test-treeshake update if that's not taken already.

Not sure what you mean, that command is working fine already. Anyway, not related to this PR.

[epreston]

Not sure what you mean, that command is working fine already. Anyway, not related to this PR.

The way it's setup it does not follow the builds, excludes some code, which reduces the accuracy of the report in this PR.

[marcofugaro]

The way it's setup it does not follow the builds, excludes some code, which reduces the accuracy of the report in this PR.

It reads from the build/ folder, but sure, go ahead.

[marcofugaro]

Alright, I've updated it to report the normal bundle size as well.

[epreston]

The difference is subtle, boring, but important. Here's a summary of what I'm pointing out.

The first report: bundle size.

You want to report on module bundle size and impacts that would be shipped. People can plainly see its 1.1 MB looking at the built file. Adding a terser step to minify this to be able to report 600 KB will confuse people. Reporting like this doesn't match up to any shipped file, it does not indicate the build size of any use case, it's an ESM build which shouldn't be minified but has, the current minimised file is based on UMD.

The final bundler is responsible for minification for good reasons. Chains of minification produce worse results on the final product, etc. It is very helpful when you introduce a new feature that adds 20 KB then are able to point out that it will not impact build sizes with the report that follows. We want to see that 20 KB here, not see it after tree-shake.

The second report: bundle size after tree shaking

This should combine the module build and module test file (not the source based test file) in a build, minify, tree-shake. Everything a real application consumer would do, then report on that. That looks like what it is still doing after this PR.

We don't want to minify the module bundle, then build it with the module test rig, minify again, and tree-shake. The minification from the first transformation can hide minifications from the final pass. We want it to all happen in a specific context of the test rig to get results that match what will happen in when we use it in our applications.

This PR doesn't modify the test rigs so its not creating any issues. The module test rig is still pointing to the build/three.module.js file because it's using the 'import' statement. I'm writing this out for clarity.

What I wanted to also point out is the current treeshake test rig has a few issues. It is too minimal to be practical. It's in a state that answers a very specific question from whoever was working on it last. It is not setup to be a general indication. I think this report and PR will have the most value with better data.

[marcofugaro]

Not sure what you're trying to explain (I got the terser part), but it doesn't look related to this PR. Let's move the conversation elsewhere.

[epreston]

Pretty easy to see its related to this PR. I am pointing out the lines in this PR where you are inventing a build type that will never ship and reporting on it.

I'm trying to capture that the test rig this report relies on is not high quality. I want people to understand this PR is good. Any issues in the data are related to the quality of the test rig, not this PR.

We just need a small modification on the first report.

[marcofugaro]

Answered in #25615 (comment), ultimately it's @mrdoob decision.

[epreston]

Work with me here, I'm trying to help. You have a great setup pointing at the wrong thing.

You haven't answered anything in that comment. You repeated something that that applies to applications not ESM libraries. It's not done by the three.js builds or any modern tools on ESM library projects for good reasons.

I want to see your work merged as soon as possible so I am quickly pointing out any issue I see. You want to report on the minified build, call it bundle size, but it does not represent the minified build or ESM bundle size. What choice are you giving people ?

This special build type you've created for "bundle size" reporting will never ship and has nothing to do with the tree-shaking test rig. Find a different home for it ? If it has a special meaning in some other context, I'm good with that. Maybe it just needs a different label ? It doesn't describe what the bundle size will be or help someone understand what the impacts of a change will be on minification. It's 200 KB out. I'm hoping for something that will show 5 KB differences accurately.

[donmccurdy]

Maybe just this function, instead of a new dependency?

export function formatBytes(bytes: number, decimals = 2): string {
	if (bytes === 0) return '0 Bytes';

	const k = 1000;
	const dm = decimals < 0 ? 0 : decimals;
	const sizes = ['Bytes', 'KB', 'MB'];

	const i = Math.floor(Math.log(bytes) / Math.log(k));

	return parseFloat((bytes / Math.pow(k, i)).toFixed(dm)) + ' ' + sizes[i];
}

[mrdoob]

Less dependencies is good 👍

[marcofugaro]

Thanks for the function @donmccurdy! Updated.

[donmccurdy]

@marcofugaro I agree with reporting the minified and gzipped ESM size, but I don't think we can put a large header into PRs that says "Bundle size after tree-shaking" like this. I expect it will mislead people into thinking we can predict average tree-shaking results.

Is there another way to format this? For example, creating a tree-shaken build that just does import { REVISION } from 'three'; and having a boolean pass/fail check if the resulting size exceeds some hard-coded threshold. We'd report gzipped size of the full ESM bundle, but not report size of a tree-shaken bundle. Is that possible and reasonable?

[epreston]

@donmccurdy what is a pre-minified ESM size supposed to be or mean ? Why does no library ship such a thing ? What is it supposed to represent if it will never be a shipped build ?

We are not predicting the average tree-shaking results. This is a sanity check against the build size of a minimum viable application using the library. The problem with that suggestion is it's not designed to fail when there is an issue. It suffers from the same problem that it does not have any meaning and does not inform reviewers of the impacts of merging a change (or if some investigation / refinement is required.

#25595 is not the best example but it is what this sort of thing should catch.

[donmccurdy]

@epreston I expect you've seen badges like this on GitHub repository READMEs before:

We are communicating the expected weight in the end-user's bundle, not size on npm's servers. That means reporting gzipped minified size, regardless of whether we publish minified code to npm.

We are not predicting the average tree-shaking results.

I do understand that, but the message added to PRs by this change suggests otherwise, and needs to be rephrased.

This is a sanity check against the build size of a minimum viable application using the library. The problem with that suggestion is it's not designed to fail when there is an issue...

That's precisely what my suggestion is designed to do. If it doesn't do so, then I'm open to alternatives. My main point is that the comment shown in PRs should communicate to the reader that "tree shaking did not break", but it currently communicates something else instead.

[epreston]

There can be value in both of these reports.

• A report on the changes to what is shipped = instant high / low risk assessment.
• A report on minimum viable tree-shake = will this impact every user of the library

[donmccurdy]

I would prefer not to report a minimum viable tree-shake size on every PR, but to simply report that tree-shaking has or has not broken. Minified, gzipped size of the full ESM bundle is OK with me too. If someone can explain to me why the "minimum viable tree-shake" is a particularly meaningful subset of the library I am open to reconsidering that!

[epreston]

@donmccurdy I have seen those. Usually they represent a number that can be associated to a shipped build product. I'm trying to point out that this is a number out of thin air. You are highlighting something that makes sense in the context of CJS or UMD when minified. It is not the same in ESM. Users will see a report of 600 kB, builds cost will actually be 418 kB, users linking via CDN will download 1.1 MB.

[donmccurdy]

@epreston it is not a good use of our time to argue about this. Reporting minified and gzipped bundle size is a widely accepted way to communicate the approximate size of a JavaScript library, whether it is imported from a CJS or ESM entrypoint.

Example: https://bundlephobia.com/package/three@0.150.1

[epreston]

No trying to argue, I'm trying to get you what you actually want and avoid what looks a little dishonest (a report only build minification) from the outside. You nailed the issue. Report on the minified CJS build product that actually ships if you need to report a minified size. It will highlight important issues and has practical meaning.

Each of these examples you are providing are reporting on something real. None of them are creating something inaccessible, only for reporting purposes, like this is.

[epreston]

If someone can explain to me why the "minimum viable tree-shake" is a particularly meaningful subset of the library I am open to reconsidering that!

What is it ?

It is an end-to-end test on build formatting and code structure. It should remain constant or slowly reduce over time. Marked increases to this size should be considered very carefully.

What's the value ?

Code can be correct and functional but have a usage penalty for the consumer of the library. Recently 10 lines of a change increased minimum build size by 4x.

(full disclosure: I don't think the claims are fair but it still needs to be addressed because it is a size-effect in a library marked side-effect free).

Why a minimum viable application ?

I'm going to use the example above to explain. The build product they were creating is dubious at best with no practical usage. I can cherry pick 2 classes and claim 10x increases. I can also pick two others and show a 3x reduction. Instead of importing a constant or cherry picking classes, grab something that is meaningful and diverse enough that it can not be disputed. It should have enough room to show both improvements and regressions.

Who does it benefit or inform ?

When someone is fairly comparing libraries with their spinning cube test, this is the number that they will use for comparison. It's the inescapable cost. It's perceptions, expectations, and a quality indicator. It's an awareness that will benefit this project greatly. Authors of changes may consider other strategies. Reviews will understand the impacts to all users.

[donmccurdy]

What I mean is, which classes from three.js does the "minimum viable tree-shake" include, and why? If this concept will mentioned under a bold header in every new PR, it should be clear to readers what we mean by it. If it's the size of a spinning cube demo, then let's say that.

---

We expect and recommend that users choose ESM. Please, let's use minified and gzipped ESM for reporting the size of the full library. That minified ESM isn't what's hosted on NPM does not matter — nothing on NPM is gzipped, either. We're talking past each other on this issue, and wasting time that could be spent productively.

[epreston]

It's the smallest practical use case that still justifies using the library. For me, "minimum viable tree-shake" is rendering the cube without animation (no spin, its reasonable to expect to pay for animation like all other alternatives) from the example on the front page of the project. Yes, cube demo.

I'm looking forward to the reality check. Right now, the value proposition is not strong unless the consumer is building something moderately substantial.

[donmccurdy]

I guess this is the list of imported classes: WebGLRenderer, PerspectiveCamera, and Scene

https://github.com/mrdoob/three.js/blob/dev/test/treeshake/index.js

@marcofugaro perhaps under that CI comment header we just add the line:

Includes a renderer, camera, and empty scene.

[epreston]

That minified ESM isn't what's hosted on NPM does not matter — nothing on NPM is gzipped, either. We're talking past each other on this issue, and wasting time that could be spent productively.

An opinion I can accept, pretending its equivalent to the those other examples will cause issues. I've done my best to warn people off doing special "reporting only" modifications to fudge numbers.

[donmccurdy]

We are not fudging numbers. This is exactly how third-party measurement tools like bundlephobia are going to report. They take the ESM entrypoint, minify and gzip it, then report the size. The goal is to measure the library in a standard way, so comparisons between packages are apples-to-apples.

[epreston]

Worth looking at that closer if that is the goal. "bundlephobia" the beta site that has an error building three.js might not be the best example. It is certainly not standard or meaningful.

[mrdoob]

Ugh... What happened here...
There goes my next 30 minutes.
/me reads...

[mrdoob]

Okay, I think I got the gist of it...

@epreston you said this:

I want to see your work merged as soon as possible so I am quickly pointing out any issue I see.

Pointing out issues is a thing. Trying so hard to convince the contributor is another thing.

Your current approach achieves two things:

1. The PR can't be merged straight away because I need to read the conversation. From 30 seconds merge to 30 minute merge (or maybe days if I don't have the energy to read the conversation).
2. Contributors get burned out because a simple PR turned into a draining dispute. They will think twice before contributing again.

We really appreciate your willingness to help but let me suggest a different approach:

1. If something in a PR is not quite right in your opinion, feel free to give feedback.
2. If the author of the PR doesn't agree, please don't try to convince them.
3. Instead, create a new PR once it is merged with the change you want .

If we disagree with the change we can discuss it in your own PR, but that way you don't block someone else's PR.

❤

[marcofugaro]

Thanks for diffusing the situation @mrdoob ❤️

@marcofugaro perhaps under that CI comment header we just add the line:
Includes a renderer, camera, and empty scene.

Done! Thanks for the suggestion @donmccurdy. I agree it was not that clear what "size after tree-shaking" meant.
However I still think reporting that size is useful. It tells the user how much they get with a basic example. It's been useful on twitter as well.

Although yes as you said this still doesn't report if the WebGLRenderer itself is tree-shakeable, even if this situation is rare, it's still the case for some users (see #25595).
The solution you posted is a good starting point, we could add a message with "✅ WebGLRenderer is tree-shakeable", or do you have better phrasing?

But I can do this in a subsequent PR, we can merge this one if we want to test the feature!

[donmccurdy]

@marcofugaro I'm good with that, thanks! I'll wait and see how the PR comment looks "in action" and then try a PR for the message if it still seems like that would be helpful.

[marcofugaro]

@donmccurdy you can see it in action in #25599