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

feat(build): cleaner logs output #10895

Merged
merged 1 commit into from
Nov 22, 2022
Merged

feat(build): cleaner logs output #10895

merged 1 commit into from
Nov 22, 2022

Conversation

ArnaudBarre
Copy link
Member

There are two improvements that goes together:

  • Don't display source map on a separated line. This create a lot of noise for something that is not downloaded by the user.
  • Sort chunks based on the size and align numbers to the right (and having lines for map would create a lot more noise here)

Before:
Screenshot 2022-11-12 at 16 00 20

Note: IMO map size should not be in yellow.

After:
Screenshot 2022-11-12 at 17 10 05

Note: This is clearly not a carefully optimised webapp, but this new report allow to quickly see images that could be compressed.

Example with chunk size limit & compress size:
Screenshot 2022-11-12 at 17 28 28

Note: In Rollup 3 maps have their own assets entries, so they are currently displayed twice in v4
Screenshot 2022-11-12 at 17 33 13

Open question: I would prefer to display size in kB instead of KiB. Is there any reason for using the later?

@patak-dev
Copy link
Member

I like this. At first, about the order from smaller to bigger, my first reaction was to want the opposite but maybe it makes sense to see the biggest files near the prompt. So I think the order is fine.

Maybe we could group JS, CSS, and assets files though? I think it is easier to parse if we see them separately, and between each group, we could order them by size.

I'm unsure though if some people would prefer to see an alphabetic list, I think it is worth adding this PR to the team board so we can discuss it with the rest. Maybe we need an option here.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 12, 2022
@benmccann
Copy link
Collaborator

I like this.

I was debating the separator (e.g. maybe - instead of /), but don't have strong feelings about it.

The grouping idea is an interesting question. I can see arguments either way depending on the scenario. Maybe I just want to optimize load speed regardless of file type and there's chances to do that with both images and JS. Or maybe I have one page with lots of big images and so then maybe I'd want them displayed separately as they're just confined to a single page. After debating it with myself, I think I would be in favor of the grouping. A big reason in my case is that I use vite-imagetools and so I'm going to have a lot of jpg images that are larger, but I don't care so much because they're only going to be served in legacy cases and most users will get avif and webp, so including the images would just meaninglessly clutter the results.

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Nov 16, 2022

Various experiment with groups, - separator, kB and dimmed assets dir:

Screenshot 2022-11-16 at 02 29 45 Screenshot 2022-11-16 at 02 50 40

@patak-dev
Copy link
Member

I like - as a separator, it feels less noisy. And I love the grouping ❤️
@ArnaudBarre I fear that seeing so many numbers on the same line may be a bit overwhelming though. Maybe we should bold the main unzipped number? (so the number for gzip and map ends up feeling a bit dimmed, as some kind of extra information?)

@ArnaudBarre
Copy link
Member Author

Will try to bold the main size. Will also try a "table" layout to avoid repeating "gzip" and "map" now that we have a headers line

@bluwy
Copy link
Member

bluwy commented Nov 16, 2022

I like the - separator too. I'm not so sure about the group title since the colors are already grouping them (if we sort by groups). Do we also want to show the gzip size for sourcemaps too? Though that would make the line quite long

@ArnaudBarre
Copy link
Member Author

Update:
Screenshot 2022-11-17 at 09 37 58

@Shinigami92
Copy link
Member

Two questions:

  1. If we only use kB, should we add 1000-separators? Like 3,550.63 kB?
  2. Is it possible to add a summary that sum up all kBs? I would like to know the full target/dist size before I deploy to production. (Not sure if in scope of this PR)

@ArnaudBarre
Copy link
Member Author

  • Why not for thousand separators 👍. Also, as asked by @patak-dev I will do a separate PR to discuss KiB -> kB
  • For full dist size it requires to know the size of copy pasted public folder, so it would require a separate sys call. Probably better in another PR

@patak-dev
Copy link
Member

I like the idea of a summary, maybe more interesting even if it is per group. I think we should try first to merge this PR with groups only so we separate the discussions.

@patak-dev patak-dev added this to the 4.0 milestone Nov 17, 2022
@thebanjomatic
Copy link
Contributor

Just my 2 cents, but I think I would prefer a vertical-pipe | instead instead of hyphen - as the separator. At first glance the hyphen looked like a minus sign and I was thinking we were doing math on the sizes.

@benmccann
Copy link
Collaborator

| might not be bad. It could make it look a little like a table, which could possibly be good. It'd be interesting to see what that looks like

@thebanjomatic
Copy link
Contributor

thebanjomatic commented Nov 21, 2022

In that case you could also try \u2502 for the "Box Drawings Light Vertical" character which should join together better between multiple lines in the terminal.

image

vs

image

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Nov 22, 2022

Rebased on main with display from #10982

I like the stronger separation of

Screenshot 2022-11-22 at 15 05 02 Screenshot 2022-11-22 at 15 06 54

@patak-dev
Copy link
Member

I also like more. I think we should leave a blank line between each group though (without a label). I think it would help while parsing the list of files and could also make the changes in columns less surprising.

@ArnaudBarre
Copy link
Member Author

The issue with a blank line is that for very small projects it looks strange
Screenshot 2022-11-22 at 16 23 14

@patak-dev patak-dev requested a review from bluwy November 22, 2022 15:38
@patak-dev
Copy link
Member

Good point @ArnaudBarre, better to leave it without the extra blank line 👍🏼
@bluwy I think you can merge it if you approve it

@ydcjeff
Copy link
Contributor

ydcjeff commented Nov 22, 2022

Like the log redesign ❤️. Just thinking out loud here, should we display JS output files in yellow-ish color like JS logo color, CSS output files in blue-ish color (CSS 3 logo color), and assets files in green? So we could separate JS, CSS, asset files with color and groups. But, I am afraid that yellow-ish JS output files could create a lot of noise since yellow-ish color is similar to warning color?

@patak-dev
Copy link
Member

@ydcjeff I think playing with the colors is an interesting idea! Let's merge this PR first and then we can create a PR that is only about the colors for each group so we better document the rationale there and don't block this one that is ready to be merged.

@patak-dev patak-dev merged commit 7d24b5f into vitejs:main Nov 22, 2022
@ArnaudBarre ArnaudBarre deleted the build-output branch November 22, 2022 21:15
@ArnaudBarre
Copy link
Member Author

Yeah the yellow is already used for "too big" chunks. But we could try some other colors combinations!

fc pushed a commit to fc/vite that referenced this pull request Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants