-
Notifications
You must be signed in to change notification settings - Fork 35
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
docs: OEP-67 automated bundle size checking. #515
Conversation
cfcf970
to
a108d20
Compare
a108d20
to
73eae79
Compare
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.
As is, these changes LGTM! Left a few non-blocking additional comments/thoughts.
|
||
It is important for users of the Open edX platform that we deliver reasonably sized JavaScript bundles. This provides | ||
faster load times to all users, and is vital for users with low bandwidth and/or metered connections. To ensure we | ||
don't unintentionally increase the size of our JavaScript bundles, we should utilize automated bundle size monitoring. |
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.
It might also be worth a mention that increased observability of bundle size will encourage maintainers to adopt practices such as code/bundle splitting.
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.
|
||
It is important for users of the Open edX platform that we deliver reasonably sized JavaScript bundles. This provides | ||
faster load times to all users, and is vital for users with low bandwidth and/or metered connections. To ensure we | ||
don't unintentionally increase the size of our JavaScript bundles, we should utilize automated bundle size monitoring. |
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.
[curious] Is it worth mentioning that we have some support for bundle size analytics today insofar that we do generate a bundle size report via the standard Webpack configurations provided by frontend-build which can be viewed locally? E.g., when you run npm run build
, a report.html
file is generated in the dist
directory that lets developers see bundle sizes of JS assets (compressed vs. uncompressed). However, this report is not very discoverable. Exposing similar data via these CI checks makes bundle size a more prominent concern than with the tools Open edX engineers have to work with today.
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 agree it's a good idea to mention it: I myself had totally forgotten about that report!
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.
Consequence | ||
*********** | ||
|
||
`BundleWatch`_ will be adopted by the Open edX community for automatic JavaScript bundle size monitoring. |
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.
This line feels like it might belong under the "Decision" section?
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.
Consequence | ||
*********** | ||
|
||
`BundleWatch`_ will be adopted by the Open edX community for automatic JavaScript bundle size monitoring. |
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.
It might also be worth a mention whether it will be up to maintainers / owning squads of repos whether the BundleWatch CI check will be a required status check or not (i.e., informative only versus blocking PRs if exceeding certain threshold). My assumption is that we'd want this CI check to be informative only.
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.
73eae79
to
9ffa820
Compare
9ffa820
to
63a963e
Compare
368419e
to
6e4ca35
Compare
6e4ca35
to
f0beece
Compare
No description provided.