-
Couldn't load subscription status.
- 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
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a3257ae:
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 789 | 797 | 5000 | |
| BaseButton | mount | 877 | 856 | 5000 | |
| Breadcrumb | mount | 2554 | 2542 | 1000 | |
| ButtonNext | mount | 505 | 505 | 5000 | |
| Checkbox | mount | 1503 | 1493 | 5000 | |
| CheckboxBase | mount | 1265 | 1253 | 5000 | |
| ChoiceGroup | mount | 4640 | 4742 | 5000 | |
| ComboBox | mount | 980 | 985 | 1000 | |
| CommandBar | mount | 9991 | 9879 | 1000 | |
| ContextualMenu | mount | 6149 | 6241 | 1000 | |
| DefaultButton | mount | 1079 | 1077 | 5000 | |
| DetailsRow | mount | 3624 | 3631 | 5000 | |
| DetailsRowFast | mount | 3612 | 3724 | 5000 | |
| DetailsRowNoStyles | mount | 3442 | 3454 | 5000 | |
| Dialog | mount | 2146 | 2145 | 1000 | |
| DocumentCardTitle | mount | 132 | 129 | 1000 | |
| Dropdown | mount | 3171 | 3184 | 5000 | |
| FluentProviderNext | mount | 7284 | 7193 | 5000 | |
| FocusTrapZone | mount | 1788 | 1798 | 5000 | |
| FocusZone | mount | 1744 | 1740 | 5000 | |
| IconButton | mount | 1700 | 1689 | 5000 | |
| Label | mount | 333 | 351 | 5000 | |
| Layer | mount | 1756 | 1745 | 5000 | |
| Link | mount | 467 | 438 | 5000 | |
| MakeStyles | mount | 1823 | 1837 | 50000 | |
| MenuButton | mount | 1433 | 1416 | 5000 | |
| MessageBar | mount | 1992 | 2007 | 5000 | |
| Nav | mount | 3202 | 3229 | 1000 | |
| OverflowSet | mount | 1027 | 1053 | 5000 | |
| Panel | mount | 2015 | 2027 | 1000 | |
| Persona | mount | 798 | 812 | 1000 | |
| Pivot | mount | 1360 | 1374 | 1000 | |
| PrimaryButton | mount | 1239 | 1286 | 5000 | |
| Rating | mount | 7511 | 7412 | 5000 | |
| SearchBox | mount | 1287 | 1285 | 5000 | |
| Shimmer | mount | 2402 | 2507 | 5000 | |
| Slider | mount | 1881 | 1878 | 5000 | |
| SpinButton | mount | 4939 | 4874 | 5000 | |
| Spinner | mount | 411 | 410 | 5000 | |
| SplitButton | mount | 3034 | 3061 | 5000 | |
| Stack | mount | 486 | 513 | 5000 | |
| StackWithIntrinsicChildren | mount | 1530 | 1501 | 5000 | |
| StackWithTextChildren | mount | 4576 | 4432 | 5000 | |
| SwatchColorPicker | mount | 10099 | 10035 | 5000 | |
| Tabs | mount | 1375 | 1369 | 1000 | |
| TagPicker | mount | 2344 | 2365 | 5000 | |
| TeachingBubble | mount | 11739 | 11693 | 5000 | |
| Text | mount | 417 | 394 | 5000 | |
| TextField | mount | 1359 | 1318 | 5000 | |
| ThemeProvider | mount | 1151 | 1164 | 5000 | |
| ThemeProvider | virtual-rerender | 575 | 581 | 5000 | |
| Toggle | mount | 792 | 770 | 5000 | |
| buttonNative | mount | 108 | 109 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Potential regressions comparing to master
| Scenario | Current PR Ticks | Baseline Ticks | Ratio | Regression Analysis |
|---|---|---|---|---|
| AlertMinimalPerf.default | 269 | 278 | 0.97:1 | analysis |
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 158 | 137 | 1.15:1 |
| HeaderSlotsPerf.default | 792 | 728 | 1.09:1 |
| AnimationMinimalPerf.default | 422 | 396 | 1.07:1 |
| DropdownManyItemsPerf.default | 706 | 659 | 1.07:1 |
| ReactionMinimalPerf.default | 380 | 360 | 1.06:1 |
| TooltipMinimalPerf.default | 1001 | 945 | 1.06:1 |
| GridMinimalPerf.default | 342 | 326 | 1.05:1 |
| ButtonSlotsPerf.default | 530 | 511 | 1.04:1 |
| LayoutMinimalPerf.default | 369 | 355 | 1.04:1 |
| TreeWith60ListItems.default | 178 | 171 | 1.04:1 |
| ButtonMinimalPerf.default | 169 | 164 | 1.03:1 |
| CheckboxMinimalPerf.default | 2739 | 2672 | 1.03:1 |
| InputMinimalPerf.default | 1256 | 1218 | 1.03:1 |
| SkeletonMinimalPerf.default | 348 | 338 | 1.03:1 |
| TableManyItemsPerf.default | 1897 | 1845 | 1.03:1 |
| TextMinimalPerf.default | 335 | 324 | 1.03:1 |
| AvatarMinimalPerf.default | 193 | 189 | 1.02:1 |
| ListNestedPerf.default | 550 | 540 | 1.02:1 |
| LoaderMinimalPerf.default | 691 | 676 | 1.02:1 |
| PortalMinimalPerf.default | 178 | 174 | 1.02:1 |
| ProviderMergeThemesPerf.default | 1679 | 1649 | 1.02:1 |
| TableMinimalPerf.default | 396 | 388 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1688 | 1675 | 1.01:1 |
| ChatWithPopoverPerf.default | 349 | 346 | 1.01:1 |
| DropdownMinimalPerf.default | 3101 | 3069 | 1.01:1 |
| ListWith60ListItems.default | 632 | 626 | 1.01:1 |
| ProviderMinimalPerf.default | 990 | 976 | 1.01:1 |
| RadioGroupMinimalPerf.default | 432 | 429 | 1.01:1 |
| SegmentMinimalPerf.default | 350 | 347 | 1.01:1 |
| TreeMinimalPerf.default | 792 | 785 | 1.01:1 |
| DividerMinimalPerf.default | 348 | 349 | 1:1 |
| EmbedMinimalPerf.default | 4058 | 4045 | 1:1 |
| FormMinimalPerf.default | 385 | 386 | 1:1 |
| ListCommonPerf.default | 614 | 615 | 1:1 |
| MenuButtonMinimalPerf.default | 1550 | 1546 | 1:1 |
| RosterPerf.default | 1135 | 1132 | 1:1 |
| PopupMinimalPerf.default | 553 | 553 | 1:1 |
| RefMinimalPerf.default | 233 | 233 | 1:1 |
| SliderMinimalPerf.default | 1559 | 1566 | 1:1 |
| TextAreaMinimalPerf.default | 474 | 475 | 1:1 |
| CustomToolbarPrototype.default | 3834 | 3840 | 1:1 |
| VideoMinimalPerf.default | 606 | 605 | 1:1 |
| ChatMinimalPerf.default | 632 | 641 | 0.99:1 |
| DatepickerMinimalPerf.default | 5314 | 5343 | 0.99:1 |
| DialogMinimalPerf.default | 737 | 743 | 0.99:1 |
| ImageMinimalPerf.default | 363 | 368 | 0.99:1 |
| ListMinimalPerf.default | 498 | 502 | 0.99:1 |
| SplitButtonMinimalPerf.default | 3704 | 3756 | 0.99:1 |
| ToolbarMinimalPerf.default | 926 | 934 | 0.99:1 |
| AttachmentSlotsPerf.default | 1044 | 1060 | 0.98:1 |
| BoxMinimalPerf.default | 337 | 345 | 0.98:1 |
| CarouselMinimalPerf.default | 455 | 465 | 0.98:1 |
| HeaderMinimalPerf.default | 348 | 355 | 0.98:1 |
| LabelMinimalPerf.default | 368 | 374 | 0.98:1 |
| MenuMinimalPerf.default | 814 | 827 | 0.98:1 |
| StatusMinimalPerf.default | 669 | 686 | 0.98:1 |
| CardMinimalPerf.default | 526 | 545 | 0.97:1 |
| ItemLayoutMinimalPerf.default | 1182 | 1218 | 0.97:1 |
| ChatDuplicateMessagesPerf.default | 273 | 285 | 0.96:1 |
| FlexMinimalPerf.default | 270 | 280 | 0.96:1 |
| AccordionMinimalPerf.default | 140 | 147 | 0.95:1 |
| IconMinimalPerf.default | 562 | 627 | 0.9:1 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 8ac36b3197c4f34825542f4db90d9d8cbac0d38a (build) |
📊 Bundle size reportUnchanged fixtures
|
a69dcfb to
c91ff53
Compare
c91ff53 to
23ceb23
Compare
788d882 to
d99143d
Compare
329fea6 to
b279bc3
Compare
| - script: | | ||
| yarn build --no-cache $(sinceArg) | ||
| displayName: build packages | ||
| yarn lage bundle-size --no-cache --verbose $(sinceArg) |
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-size command while build is only a dependency (I also modified lage.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.)- As required artifacts are not present (for
react-image), build will fail
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.
yarn build --no-cache will run build for react-menu only
not sure this is correct -> yarn build from root will run lage build so all packages will be build (with dependant tasks )
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:
- script: |
- yarn lage bundle-size --no-cache
+ yarn lage bundle-size --no-cache $(sinceArg)
displayName: create reports for packagesBut merging will give us better concurrency as lage will run bundle-size commands immediately once all required builds are completed.
b279bc3 to
7bb9858
Compare
7bb9858 to
8f6592c
Compare
| report.push('</details>'); | ||
| } | ||
|
|
||
| report.push( |
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.
the amount of these report.push calls with string interpolation looks kinda scary, have you considered using an ejs template ?
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.
Agree 😓
As I know EJS is designed for HTML templates and is not supported by Prettier (prettier/prettier#8676 (comment)). I tried to rewrite this to EJS template and I don't think that it become more readable 😕
Do you think that we should go with ejs? Are there any other options?
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.
ejs might be good idea, but we'd lost all TS advantages....
I'd keep it within js for now, although it would be nice to make it more readable.
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.
I quickly hacked and came up with something like this
async function markdownReporter(result, commitSHA, quiet) {
const packageRoot = findPackageRoot(__dirname);
if (!packageRoot) {
throw new Error(
[
'Failed to find a package root (directory that contains "package.json" file)',
`Lookup start in: ${__dirname}`,
].join('\n'),
);
}
const artifactsDir = path.resolve(packageRoot, 'dist');
const artifactsFilename = path.join(artifactsDir, 'bundle-size.md');
const { changedEntries, unchangedEntries } = getChangedEntriesInReport(result);
const template = stripIndents`
## 📊 Bundle size report
${printChanged(changedEntries)}
${printUnchanged(unchangedEntries)}
<sub>🤖 This report was generated against <a href='https://github.com/microsoft/fluentui/commit/${commitSHA}'>${commitSHA}</a></sub>
`;
await fs.mkdir(artifactsDir, { recursive: true });
await fs.writeFile(artifactsFilename, prettier.format(template, { parser: 'markdown' }));
if (!quiet) {
console.log([chalk.blue('[i]'), `A report file was written to ${artifactsFilename}`].join(' '));
}
};
/**
*
* @param {getChangedEntriesInReport.ComparedReport} entries
*/
function printChanged(entries) {
if (entries.length > 0) {
const template = `
| Package & Exports | Baseline (minified/GZIP) | PR | Change |
| :---------------- | -----------------------: | ----: | ---------: |
${entries.map(newChangedEntry)}
`;
return template;
}
return '';
}
/**
*
* @param {getChangedEntriesInReport.ComparedReport} entries
*/
function printUnchanged(entries) {
if (entries.length > 0) {
const template = `
<details>
<summary>Unchanged fixtures</summary>
| Package & Exports | Size (minified/GZIP) |
| ----------------- | -------------------: |
${entries.map(newUnchangedEntry)}
</details>
`;
return template;
}
return '';
}
/**
*
* @param {getChangedEntriesInReport.ComparedReport[number]} entry
*/
function newUnchangedEntry(entry) {
const title = `<samp>${entry.packageName}</samp> <br /> <abbr title='${entry.path}'>${entry.name}</abbr>`;
const size = [`\`${formatBytes(entry.minifiedSize)}\``, '<br />', `\`${formatBytes(entry.gzippedSize)}\``].join('');
return `| ${title} | ${size} |`;
}
/**
*
* @param {getChangedEntriesInReport.ComparedReport[number]} entry
*/
function newChangedEntry(entry) {
const title = `<samp>${entry.packageName}</samp> <br /> <abbr title='${entry.path}'>${entry.name}</abbr>`;
const before = entry.diff.empty
? [`\`${formatBytes(0)}\``, '<br />', `\`${formatBytes(0)}\``].join('')
: [
`\`${formatBytes(entry.minifiedSize + entry.diff.minified.delta)}\``,
'<br />',
`\`${formatBytes(entry.gzippedSize + entry.diff.gzip.delta)}\``,
].join('');
const after = [`\`${formatBytes(entry.minifiedSize)}\``, '<br />', `\`${formatBytes(entry.gzippedSize)}\``].join('');
const difference = entry.diff.empty
? '🆕 New entry'
: [`${formatDelta(entry.diff.minified)}`, '<br />', `${formatDelta(entry.diff.gzip)}`].join('');
return `| ${title} | ${before} | ${after} | ${difference}|`;
}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.
I am not sure that it improves something drastically, but I can apply it if you want ¯_(ツ)_/¯
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.
if ejs does not seem to improve smth then we can keep it like this. My thought was that it would remove the need for all the escaping and you could just pass a data object to tokenize the content
fccc736 to
3dcca65
Compare
642bc19 to
d6161d5
Compare
…hore/bundle-size-report � Conflicts: � azure-pipelines.bundlesize.yml
| const sampleComparedReport = require('../../__fixture__/sampleComparedReport'); | ||
|
|
||
| function noop() { | ||
| /* does nothing */ |
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.
| /* does nothing */ |
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 so fast 😥
$ eslint . --ext .js
office-ui-fabric-react\packages\bundle-size\src\reporters\cliReporter.test.js
6:17 error Unexpected empty function 'noop' @typescript-eslint/no-empty-function
✖ 1 problem (1 error, 0 warnings)
| /** | ||
| * @param {import("../utils/collectLocalReport").BundleSizeReportEntry} local | ||
| * @param {import("../utils/collectLocalReport").BundleSizeReportEntry} remote | ||
| * @param {'minifiedSize' | 'gzippedSize'} property |
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.
I didn't meant that it makes it makes things worse :)
we will have more fields
I don't see this making any difference, but whatever works !
| /** @typedef {import('./calculateDiffByMetric').DiffByMetric} DiffByMetric */ | ||
| /** @typedef {{ empty: boolean, minified: DiffByMetric, gzip: DiffByMetric }} DiffForEntry */ | ||
|
|
||
| /** @typedef {import("./collectLocalReport").BundleSizeReportEntry & { diff: DiffForEntry }} ComparedReportEntry */ |
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 are using this on 3 places -> makes sense to create local type via typedef
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.
Didn't get this suggestion: it's already a local typedef...
| const sampleComparedReport = require('../../__fixture__/sampleComparedReport'); | ||
| const { handler } = require('./compareReports'); | ||
|
|
||
| describe('compareReports', () => { |
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 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!
| expect(remoteReport).toEqual(sampleReport); | ||
| }); | ||
|
|
||
| it('retries to fetch a report', async () => { |
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.
nit: max n of retries is not covered by UTs :-P
* chore: integrate new bundle size tool * move getChangedEntriesInReport() to a separate file, reuse it in both reporters * Update packages/bundle-size/src/commands/compareReports.js Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update packages/bundle-size/src/reporters/markdownReporter.js Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * fix snapshot * do not mock chalk * add comment * update test * use Object.freeze, add tests for getChangedEntriesInReport * update cliReporter to exit early * refactor, do not use tuple, extract const * use snapshot serializer and strip-ansi * rename variables to clarify usage * refactor getRemoteReport * move sampleReport to __fixture__ * add comment to fix lint error * refactor getDirectionSymbol * refactor calculateDiffByMetric * use toEqual instead of snapshots * handle zero values * move url to const * add tests for compareReports & getRemoteReport * exclude tests from commands * fix regex Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Pull request checklist
Include a change request file using$ yarn changeDescription of changes
This step adds bundle size comparison for CI and local runs 🚀 Functionality is implemented via an additional command that generates report for CLI & in Markdown based on existing artifacts (#18322):
Reporting reuses our existing
GithubPRCommenttask. A sample markdown report can be viewed directly on this PR, #18256 (comment).Sample CLI output:
For remote data we are using data that are uploaded during
masterbuilds, see #18322.Failures on increases will be implemented on later, current step contains only reporting.