-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 check to react-northstar build #16686
Add size check to react-northstar build #16686
Conversation
Asset size changes
Baseline commit: 6896697d21ee038e3a4921b033d5f1f68537aed0 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
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 e056ee6:
|
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.
Thanks for doing this but since size auditor is already our longest build, we'll need to split the v0 and v8 parts into separate parallel jobs for this to be feasible. Will comment back with specific instructions later.
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.
Hi @ecraig12345 the webpack upgrade actually breaks the size-auditor. I pushed new changes that should fix it for fabric + added N* components in it. Splitting it into separate jobs would be welcome for reducing the time |
Ah okay... 😬 @dzearing should take a look at your changes since he did the webpack upgrade. |
Here's what needs to happen to split the jobs. A couple easy parts first:
(As an alternative to both of these, I guess you could add a separate package Next split up the jobs:
- job: build-react
timeoutInMinutes: 75
pool:
vmImage: 'windows-2019'
steps:
- task: NodeTool@0
inputs:
versionSpec: '12.x'
displayName: 'Install Node.js'
- script: npx midgard-yarn install
displayName: yarn
- script: yarn build --to @fluentui/react @fluentui/react-button @fluentui/react-compose --no-cache
displayName: yarn build to @fluentui/react
- script: yarn workspace test-bundles bundle:size --package @fluentui/react
displayName: yarn bundle test-bundles
- script: yarn bundlesizecollect
displayName: 'Collate Bundle Size Information'
- task: PublishBuildArtifacts@1
displayName: 'Publish Bundle Size information to Azure Dev Ops Artifacts'
inputs:
PathtoPublish: 'apps/test-bundles/dist/bundlesizes.json'
ArtifactName: bundlesizes-react
- task: PublishBuildArtifacts@1
displayName: 'Publish Artifact dist folder upon build for debug'
inputs:
PathtoPublish: 'apps/test-bundles/dist'
ArtifactName: distdrop-react
- job: build-northstar
timeoutInMinutes: 75
pool:
vmImage: 'windows-2019'
steps:
- task: NodeTool@0
inputs:
versionSpec: '12.x'
displayName: 'Install Node.js'
- script: npx midgard-yarn install
displayName: yarn
- script: yarn build --to @fluentui/react-northstar --no-cache
displayName: yarn build to @fluentui/react-northstar
- script: yarn workspace test-bundles bundle:size --package @fluentui/react-northstar
displayName: yarn bundle test-bundles
- script: yarn bundlesizecollect
displayName: 'Collate Bundle Size Information'
- task: PublishBuildArtifacts@1
displayName: 'Publish Bundle Size information to Azure Dev Ops Artifacts'
inputs:
PathtoPublish: 'apps/test-bundles/dist/bundlesizes.json'
ArtifactName: bundlesizes-northstar
- task: PublishBuildArtifacts@1
displayName: 'Publish Artifact dist folder upon build for debug'
inputs:
PathtoPublish: 'apps/test-bundles/dist'
ArtifactName: distdrop-northstar The next part is the tough one. Currently the actual size auditor back end (what the Option 1: add a merging task to the
|
Commenter does not have sufficient privileges for PR 16686 in repo microsoft/fluentui |
…-react-n-build # Conflicts: # apps/test-bundles/package.json
@ecraig12345 It seems that the lightrail job is taking the bundlesizes.json from the build_react job and not the bundlesizes.json from the merge job that contains React + React N* sizes. Do you know how does the lightrail work or how is it configured? I was looking at the lightrail pipeline and found only this: devopsDropFolderName : drop This should be the last thing to solve hopefully. Thanks again for all your help |
…-react-n-build # Conflicts: # apps/test-bundles/package.json
…-react-n-build # Conflicts: # apps/test-bundles/package.json
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 don't have any comments, great job 👍
Pull request checklist
$ yarn change
Description of changes
Bundle size auditor for Fabric is broken since the update to Webpack 5 due to the Tree shaking. This PR is fixing this by creating new file with component import and console log + adding N* to the auditor check.
(give an overview)
Focus areas to test
(optional)