-
Notifications
You must be signed in to change notification settings - Fork 2.8k
chore: integrate new bundle size tool #18256
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
Changes from all commits
8f6592c
3687430
d440307
942a4aa
dcb1e97
3dcca65
86bb6b8
307083b
fbd16bc
6eeaf81
17d9a86
9b30a82
217031c
e150032
859b054
bd561fd
1cc78db
ddd0ec5
1017421
d6161d5
6bdf2cd
0f0b498
190e052
e30d276
a3257ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,14 @@ export default { | |
|
|
||
| ### Commands | ||
|
|
||
| #### `compare-reports` | ||
|
|
||
| Compares local (requires call of `bundle-size measure` first) and remote results, provides output to CLI or to a Markdown file. | ||
|
|
||
| ```sh | ||
| yarn bundle-size compare-reports --branch=main --output=["cli"|"markdown"] [--quiet] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we plan to support any commit not just branch ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it's not planned yet. Currently we store data only for latest build for a branch |
||
| ``` | ||
|
|
||
| #### `measure` | ||
|
|
||
| ```sh | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| const { emptyDiff } = require('../src/utils/compareResultsInReports'); | ||
|
|
||
| /** @type {import('../src/utils/compareResultsInReports').ComparedReport} */ | ||
| const sampleComparedReport = [ | ||
| { | ||
| packageName: 'foo-package', | ||
| name: 'New entry', | ||
| path: 'foo.fixture.js', | ||
| minifiedSize: 1000, | ||
| gzippedSize: 100, | ||
| diff: emptyDiff, | ||
| }, | ||
| { | ||
| packageName: 'bar-package', | ||
| name: 'An entry without diff', | ||
| path: 'bar.fixture.js', | ||
| minifiedSize: 1000, | ||
| gzippedSize: 100, | ||
| diff: { | ||
| empty: false, | ||
|
|
||
| minified: { delta: 0, percent: '0%' }, | ||
| gzip: { delta: 0, percent: '0%' }, | ||
| }, | ||
| }, | ||
| { | ||
| packageName: 'baz-package', | ||
| name: 'An entry with diff', | ||
| path: 'baz.fixture.js', | ||
| minifiedSize: 1000, | ||
| gzippedSize: 100, | ||
| diff: { | ||
| empty: false, | ||
|
|
||
| minified: { delta: 1000, percent: '100%' }, | ||
| gzip: { delta: 100, percent: '100%' }, | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| module.exports = sampleComparedReport; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** @type {import('../src/utils/collectLocalReport').BundleSizeReport} */ | ||
| const sampleReport = [ | ||
| { | ||
| packageName: 'foo-package', | ||
| name: 'New entry', | ||
| path: 'foo.fixture.js', | ||
| minifiedSize: 1000, | ||
| gzippedSize: 100, | ||
| }, | ||
| { | ||
| packageName: 'bar-package', | ||
| name: 'An entry without diff', | ||
| path: 'bar.fixture.js', | ||
| minifiedSize: 1000, | ||
| gzippedSize: 100, | ||
| }, | ||
| { | ||
| packageName: 'baz-package', | ||
| name: 'An entry with diff', | ||
| path: 'baz.fixture.js', | ||
| minifiedSize: 1000, | ||
| gzippedSize: 100, | ||
| }, | ||
| ]; | ||
|
|
||
| module.exports = sampleReport; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| const chalk = require('chalk'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have quit a lot untested code (other commands as well), can you add some for this new command, add coverage for other existing later?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added tests for this command in 0f0b498. |
||
|
|
||
| const cliReporter = require('../reporters/cliReporter'); | ||
| const markdownReporter = require('../reporters/markdownReporter'); | ||
| const collectLocalReport = require('../utils/collectLocalReport'); | ||
| const { compareResultsInReports } = require('../utils/compareResultsInReports'); | ||
| const getRemoteReport = require('../utils/getRemoteReport'); | ||
| const { hrToSeconds } = require('../utils/helpers'); | ||
|
|
||
| /** | ||
| * @param {typeof import('../index') & { branch: string, output: 'cli' | 'markdown' }} options | ||
| */ | ||
| async function compareReports(options) { | ||
| const { branch, output, quiet } = options; | ||
| const startTime = process.hrtime(); | ||
|
|
||
| const localReportStartTime = process.hrtime(); | ||
| const localReport = await collectLocalReport(); | ||
|
|
||
| if (!quiet) { | ||
| console.log( | ||
| [chalk.blue('[i]'), `Local report prepared in ${hrToSeconds(process.hrtime(localReportStartTime))}`].join(' '), | ||
| ); | ||
| } | ||
|
|
||
| const remoteReportStartTime = process.hrtime(); | ||
| const { commitSHA, remoteReport } = await getRemoteReport(branch); | ||
|
|
||
| if (!quiet) { | ||
| if (commitSHA === '') { | ||
| console.log([chalk.blue('[i]'), `Remote report for "${branch}" branch was not found`].join(' ')); | ||
| } else { | ||
| console.log( | ||
| [ | ||
| chalk.blue('[i]'), | ||
| `Remote report for "${commitSHA}" commit fetched in ${hrToSeconds(process.hrtime(remoteReportStartTime))}`, | ||
| ].join(' '), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const result = compareResultsInReports(localReport, remoteReport); | ||
|
|
||
| switch (output) { | ||
| case 'cli': | ||
| await cliReporter(result); | ||
| break; | ||
| case 'markdown': | ||
| await markdownReporter(result, commitSHA, quiet); | ||
| break; | ||
| } | ||
|
|
||
| if (!quiet) { | ||
| console.log(`Completed in ${hrToSeconds(process.hrtime(startTime))}`); | ||
| } | ||
| } | ||
|
|
||
| // --- | ||
|
|
||
| /** @type {import('yargs').CommandModule} */ | ||
| const api = { | ||
| command: 'compare-reports', | ||
| describe: 'compares local and remote results', | ||
| builder: { | ||
| branch: { | ||
| alias: 'b', | ||
| type: 'string', | ||
| description: 'A branch to compare against', | ||
| default: 'main', | ||
| }, | ||
| output: { | ||
| alias: 'o', | ||
| type: 'string', | ||
| choices: ['cli', 'markdown'], | ||
| description: 'Defines a reporter to produce output', | ||
| default: 'cli', | ||
| }, | ||
| }, | ||
| handler: compareReports, | ||
| }; | ||
|
|
||
| module.exports = api; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| const getRemoteReport = jest.fn(); | ||
| const collectLocalReport = jest.fn(); | ||
| const compareResultsInReports = jest.fn(); | ||
| const cliReporter = jest.fn(); | ||
|
|
||
| jest.mock('../reporters/cliReporter', () => cliReporter); | ||
| jest.mock('../utils/collectLocalReport', () => collectLocalReport); | ||
| jest.mock('../utils/compareResultsInReports', () => ({ compareResultsInReports })); | ||
| jest.mock('../utils/getRemoteReport', () => getRemoteReport); | ||
|
|
||
| const sampleReport = require('../../__fixture__/sampleReport'); | ||
| const sampleComparedReport = require('../../__fixture__/sampleComparedReport'); | ||
| const { handler } = require('./compareReports'); | ||
|
|
||
| describe('compareReports', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to come up with some more sophisticated e2e testing for our CLI tools, while this is definitely better than nothing, we are testing too much implementation details via mocking etc. anyways thanks you! |
||
| it('fetches remote report and compares it with a local data', async () => { | ||
| const branchName = 'master'; | ||
|
|
||
| getRemoteReport.mockImplementation(() => ({ commitSHA: 'test', remoteReport: sampleReport })); | ||
| collectLocalReport.mockImplementation(() => sampleReport); | ||
| compareResultsInReports.mockImplementation(() => sampleComparedReport); | ||
|
|
||
| await handler({ quiet: true, branch: branchName, output: 'cli' }); | ||
|
|
||
| expect(getRemoteReport).toHaveBeenCalledWith(branchName); | ||
| expect(compareResultsInReports).toHaveBeenCalledWith(sampleReport, sampleReport); | ||
| expect(cliReporter).toHaveBeenCalledWith(sampleComparedReport); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,14 @@ | ||
| const yargs = require('yargs'); | ||
|
|
||
| const cliSetup = yargs | ||
| .commandDir('commands') | ||
| .commandDir('commands', { exclude: /.test\.js$/ }) | ||
| .option('quiet', { | ||
| alias: 'q', | ||
| type: 'boolean', | ||
| description: 'Suppress verbose build output', | ||
| default: false, | ||
| }) | ||
| .scriptName('bundle-size') | ||
layershifter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .version(false).argv; | ||
|
|
||
| module.exports = cliSetup; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`markdownReporter renders a report to a file 1`] = ` | ||
| "## 📊 Bundle size report | ||
|
|
||
| | Package & Exports | Baseline (minified/GZIP) | PR | Change | | ||
| | :------------------------------------------------------------------------------------- | -----------------------: | ------------------: | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------: | | ||
| | <samp>baz-package</samp> <br /> <abbr title='baz.fixture.js'>An entry with diff</abbr> | \`2 kB\`<br />\`200 B\` | \`1 kB\`<br />\`100 B\` | \`1 kB\` <img aria-hidden=\\"true\\" src=\\"https://microsoft.github.io/sizeAuditor-website/images/icons/IncreaseYellow.svg\\" /><br />\`100 B\` <img aria-hidden=\\"true\\" src=\\"https://microsoft.github.io/sizeAuditor-website/images/icons/IncreaseYellow.svg\\" /> | | ||
| | <samp>foo-package</samp> <br /> <abbr title='foo.fixture.js'>New entry</abbr> | \`0 B\`<br />\`0 B\` | \`1 kB\`<br />\`100 B\` | 🆕 New entry | | ||
|
|
||
| <details> | ||
| <summary>Unchanged fixtures</summary> | ||
|
|
||
| | Package & Exports | Size (minified/GZIP) | | ||
| | ----------------------------------------------------------------------------------------- | -------------------: | | ||
| | <samp>bar-package</samp> <br /> <abbr title='bar.fixture.js'>An entry without diff</abbr> | \`1 kB\`<br />\`100 B\` | | ||
|
|
||
| </details> | ||
| <sub>🤖 This report was generated against <a href='https://github.com/microsoft/fluentui/commit/commit-hash'>commit-hash</a></sub> | ||
| " | ||
| `; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| const chalk = require('chalk'); | ||
| const Table = require('cli-table3'); | ||
|
|
||
| const getChangedEntriesInReport = require('../utils/getChangedEntriesInReport'); | ||
| const { formatBytes } = require('../utils/helpers'); | ||
|
|
||
| /** | ||
| * @param {number} value | ||
| * | ||
| * @return {string} | ||
| */ | ||
| function getDirectionSymbol(value) { | ||
| if (value < 0) { | ||
| return '↓'; | ||
| } | ||
|
|
||
| if (value > 0) { | ||
| return '↑'; | ||
| } | ||
|
|
||
| return ''; | ||
| } | ||
|
|
||
| /** | ||
| * @param {import('../utils/calculateDiffByMetric').DiffByMetric} diff | ||
| * | ||
| * @return {string} | ||
| */ | ||
| function formatDelta({ delta, percent }) { | ||
| if (delta === 0) { | ||
| return ''; | ||
| } | ||
|
|
||
| const colorFn = delta > 0 ? chalk.red : chalk.green; | ||
|
|
||
| return colorFn(percent + getDirectionSymbol(delta)); | ||
| } | ||
|
|
||
| /** | ||
| * @param {import('../utils/compareResultsInReports').ComparedReport} report | ||
| */ | ||
| module.exports = async function cliReporter(report) { | ||
| const result = new Table({ | ||
| colAligns: ['left', 'right', 'right'], | ||
| head: ['Fixture', 'Before', 'After (minified/GZIP)'], | ||
| }); | ||
| const { changedEntries } = getChangedEntriesInReport(report); | ||
|
|
||
| changedEntries.forEach(entry => { | ||
| const { diff, gzippedSize, minifiedSize, name, packageName } = entry; | ||
| const fixtureColumn = chalk.bold(packageName) + '\n' + name + (diff.empty ? chalk.cyan(' (new)') : ''); | ||
|
|
||
| const minifiedBefore = diff.empty ? 'N/A' : formatBytes(minifiedSize + diff.minified.delta); | ||
| const gzippedBefore = diff.empty ? 'N/A' : formatBytes(gzippedSize + diff.gzip.delta); | ||
|
|
||
| const minifiedAfter = formatBytes(minifiedSize); | ||
| const gzippedAfter = formatBytes(gzippedSize); | ||
|
|
||
| const beforeColumn = minifiedBefore + '\n' + gzippedBefore; | ||
| const afterColumn = | ||
| formatDelta(diff.minified) + ' ' + minifiedAfter + '\n' + formatDelta(diff.gzip) + ' ' + gzippedAfter; | ||
|
|
||
| result.push([fixtureColumn, beforeColumn, afterColumn]); | ||
| }); | ||
|
|
||
| if (result.length > 0) { | ||
| console.log(result.toString()); | ||
| return; | ||
| } | ||
|
|
||
| console.log(`${chalk.green('[✔]')} No changes found`); | ||
| }; |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should run
bundle-sizecommand whilebuildis only a dependency (I also modifiedlage.config.js) otherwise it could cause build failures due scoped builds.For example:yarn build --no-cache --since=origin/masterwill run build forreact-northstaronlyyarn lage bundle-size --no-cachewill runbundle-sizefor all packages (react-menu,react-image, etc.)react-image), build will failThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is correct ->
yarn buildfrom root will runlage buildso all packages will be build (with dependant tasks )Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, it's already an issue in other PRs, see #18876 😐 (I modified initial message to make an example more obvious).
This could be solved without merging into one command:
But merging will give us better concurrency as
lagewill runbundle-sizecommands immediately once all requiredbuilds are completed.