Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add size snapshot #4

Closed
wants to merge 2 commits into from
Closed

Add size snapshot #4

wants to merge 2 commits into from

Conversation

TrySound
Copy link
Contributor

This plugin shows a few different sizes and provides treeshakability
information (some more stuff is coming). This snapshot also can
demonstrate users how much the project will add to their bundles.

Size snapshot is generated only with build script to not freeze watch
mode.

"treeshaked": {
"rollup": {
"code": 15990,
"import_statements": 21
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this information mean? (How do users consume it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's import statements size. When code and import_statements are equal then everything is perfect. I probably need to work more on user friendliness. For now I mostly use it just to achieve treeshakability.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When code and import_statements are equal then everything is perfect.

I'm not sure I understand. The two are nowhere near equal. Can you maybe say this in another way?

To be clear, I see the value of tracking snapshot size. React does this. I think it would be nice to even setup a bot that comments on PRs showing how much each PR would impact the size (also like React does).

I'm just not sure I understand this "treeshaked" info.

(I'm not even sure about the term "treeshaked". Is that even a real term? 😁)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to show how big will be the bundle if you import nothing. Practically this mean a perfect bundle without toplevel side effects. I achieve this via bundling import {} from 'lib' entry point. The result size is provided for both rollup and webpack.

code means the size of treeshaked bundle
import_statements means the size of all import blaBla from 'bla-bla' statements which are not treeshaaked by default being external.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think I should replace code to something different? Maybe bundle?

Copy link
Contributor Author

@TrySound TrySound May 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iife problem can be solved with uglify dead code elimination which is able to remove function calls with /*#__PURE__*/ annotation.

This is the problem of react only right now. In the future Babel should fix this issue by moving static properties inside iife.

My solution right now is wrapping with pure iife before transpilation.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the strategy for using the built snapshot JSON in this repo?

Is the file committed to master by CI, and compared for branches (but not committed)? Or...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is machSnapshot option which fails if generated snapshot does not match existing one with optional threshold. With CI this forces users to commit new size. Maybe bot is good too but I never worked with them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary of a programmatic threshold. I like the idea of a bot that comments on a PR with a diff (like React does currently) and then a maintainer can decide if the feature is worth the delta.

Copy link
Contributor Author

@TrySound TrySound May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't like threshold too but added because of a couple of requests.

The idea is similar but instead of bot snapshot is committed by user. And user can be forced by CI. Threshold is 0 by default so user is always forced to update snapshots.

@bvaughn
Copy link
Owner

bvaughn commented May 26, 2018

Rebase or merge changes from master if you don't mind. I realized my Prettier glob pattern wasn't working right. Had meant to fix that earlier but...

@TrySound
Copy link
Contributor Author

Done

@TrySound
Copy link
Contributor Author

So what do you decide? I need to track treeshaked size to see improvements.

@bvaughn
Copy link
Owner

bvaughn commented May 30, 2018

I still feel the same way as before.

I don't see the usefulness of this change without a CI+PR bot strategy.

And I'm not comfortable with a programmatic threshold. I think it's useful info but a maintainer needs to make the call.

I do think it's useful to track over time though.

This plugin shows a few different sizes and provides treeshakability
information (some more stuff is coming). This snapshot also can
demonstrate users how much the project will add to their bundles.

Size snapshot is generated only with build script to not freeze watch
mode.
@TrySound
Copy link
Contributor Author

Take a look at the latest snapshot. This is what I wanted to achieve with visible results.

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2018

I understand the usefulness of such a metric 😄 React uses a similar approach to measure the impact of changes on build size (with a bot that comments on PRs). I'd love to have something like that for react-window.

My concern all along, as I've mentioned, is that I'm not comfortable with a programmatic threshold that fails the build (i.e. the machSnapshot option). The information should be highly visible, but a maintainer should make the call about whether to merge or not.

I guess the end-to-end story of how this would work is a bit unclear to me. At the end of our last thread, you said:

The idea is similar but instead of bot snapshot is committed by user. And user can be forced by CI. Threshold is 0 by default so user is always forced to update snapshots.

I think the ergonomics of relying on / requiring users to submit this file seems less than ideal. It would also cause merge conflicts all over the place any time more than one PR is opened. I prefer the approach React uses where CI runs the task locally and leaves a comment with the delta. Would something like that be possible?

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2018

Ideally, we would also measure the impact of PRs like #25 which strip code from production bundles as well.

@TrySound
Copy link
Contributor Author

Production/development sizes is a good idea.

I already started investigation about bots. And still I would like to make its result visible in one place so users could rely on it choosing library. Do you have ideas where from bot perspective?

@TrySound
Copy link
Contributor Author

Could you submit an issue about dev prod sizes?

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2018

I already started investigation about bots. And still I would like to make its result visible in one place so users could rely on it choosing library. Do you have ideas where from bot perspective?

Interesting. Sounds like you're talking about a website (similar to bundle phobia) that shows the results for various packages, where as I've been focusing on how a maintainer would use this data to avoid regressions.

Could you submit an issue about dev prod sizes?

Sure! TrySound/rollup-plugin-size-snapshot#20

@TrySound
Copy link
Contributor Author

Yep, probably a link in readme, or regenerated image could help here.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants