Skip to content

Add Bundle Report #378

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 6, 2018
Merged

Add Bundle Report #378

merged 3 commits into from
Sep 6, 2018

Conversation

@jakub300
Copy link
Collaborator

@jakub300 jakub300 commented Jun 7, 2018 •
edited
Loading

Fixes #325

@jakub300 jakub300 changed the title WIP: Add Bundle Report Add Bundle Report Aug 16, 2018
@jakub300
Copy link
Collaborator Author

jakub300 commented Aug 16, 2018

Hey, this is ready. It adds two commands yarn report and yarn build-report.

@arturkot
Copy link
Contributor

arturkot commented Aug 20, 2018

Hi,

I like this addition! Just wondering if we need yarn report – it seems to show what's currently in dist: might be unminified dev files or unminified + the last build (could be out of date). Seems to be a bit confusing IMHO. yarn build-report, on the other hand, is what I really want. The size of production ready files. Perhaps we should leave just yarn build-report command?

@jakub300
Copy link
Collaborator Author

jakub300 commented Aug 20, 2018

Done

arturkot
arturkot approved these changes Aug 20, 2018
View reviewed changes
Copy link
Contributor

@arturkot arturkot left a comment

Choose a reason for hiding this comment

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

👍

generators/app/templates/package.json Outdated
"build-report": "gulp build && gulp report",
"dev": "gulp",
"lint": "gulp lint-js && gulp lint-css",
"report": "gulp report",
Copy link
Contributor

@arturkot arturkot Aug 20, 2018

Choose a reason for hiding this comment

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

Well, that was quick. 😅

arturkot
arturkot reviewed Aug 20, 2018
View reviewed changes
generators/app/templates/gulp/tasks/report.js
const os = require('os');
const opn = require('opn');

const MEBIBYTE = 1024 * 1024;
Copy link
Contributor

@arturkot arturkot Aug 20, 2018

Choose a reason for hiding this comment

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

MEGABYTE? :)

Copy link
Collaborator Author

@jakub300 jakub300 Aug 20, 2018

Choose a reason for hiding this comment

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

I wanted to be precise, mega while more often used is confusing.

Copy link
Contributor

@arturkot arturkot Aug 20, 2018

Choose a reason for hiding this comment

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

Got it. Should've asked Google before writing. ;) https://en.wikipedia.org/wiki/Mebibyte

arturkot
arturkot reviewed Aug 20, 2018
View reviewed changes
generators/app/templates/gulp/tasks/report.js Outdated

Object.keys(files)
.sort()
.forEach(link => {
Copy link
Contributor

@arturkot arturkot Aug 20, 2018

Choose a reason for hiding this comment

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

Hmm. Maybe .map instead of pushing using forEach? You could return a string with tr. A bit less writing. Unless I missed something. 🤔

Copy link
Collaborator Author

@jakub300 jakub300 Aug 20, 2018

Choose a reason for hiding this comment

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

Yep, done.

@jakub300 jakub300 merged commit d62620e into xfiveco:master Sep 6, 2018
@jakub300 jakub300 deleted the bundle-report branch September 6, 2018 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@luboskmetko luboskmetko luboskmetko approved these changes

+1 more reviewer

@arturkot arturkot arturkot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jakub300 jakub300

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jakub300 @arturkot @luboskmetko