-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
build(webpack): Change webpack to have smaller chunks and make all entries async #25744
build(webpack): Change webpack to have smaller chunks and make all entries async #25744
Conversation
@BYK there's a chance this could affect custom plugins (this will break the assumption that |
<div id="u2f-challenge"> | ||
<div class="loading"> | ||
<div class="loading-mask"></div> | ||
<div class="loading-indicator"></div> | ||
</div> | ||
</div> |
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.
Because this component is loaded async, we need to show a loader
name: 'renderReact', | ||
component: 'U2fSign', |
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.
Slightly changed the API from #25731
These are the new changes since the initial PR: https://github.com/getsentry/sentry/pull/25744/files/966dc3990ec2e6342a9327f375d235e86726c37b..5b782112ce527872a0db18e2809fe7486d6ea124 |
@billyvg how would a plugin make use of |
@BYK For jQuery, I would recommend including the package in the plugin template like we did for For How does that sound? |
YESSSS!!!! 🥳
This works for me.
Also sounds good. I think the key thing here is, we cannot just do this overnight. We need to put warnings (at least to console or something) and make plugin users and developers aware that a change of this nature coming. We may place stubs in place if this would delay some important infra work. |
@BYK I agree, although I don't think having console warnings will be helpful, but not sure what else we can really do. Ideally I would add some metrics to this to decide how to proceed, but we can't really do that for onprem. |
Why not? We already have the beacon. If it is on (there's a config option to turn it off), we can send another beacon for this? |
@BYK Ah forgot about the beacon. It's still a decent-sized task to implement for a scenario that may or may not exist. We'd have introduce an endpoint for the frontend to trigger, and then modify the beacon endpoint itself. How would we handle the installs that don't enable the beacon? |
We won't :)
Agree with the front-end trigger but why not the front-end directly hit an endpoint over at sentry.io instead of going through the beacon. I gave beacon as an example for data collection from self-hosted when needed. We don't have to use that directly if it makes things more complicated. |
We could do it direct from frontend, but we would need to expose the beacon configuration to the user/frontend. |
We currently have a "beacon" option that on-premise users can set that allows their Sentry install to ping our SaaS. This piggybacks off the beacon option and adds an API endpoint to allow the frontend to potentially track metrics back to our SaaS beacon endpoint. We will use this to figure out the number of installs that are using APIs that we would like to deprecate. See #25744 for a discussion around this.
Not sure how hard that would be. I'll leave this to you. |
We currently have a "beacon" option that on-premise users can set that allows their Sentry install to ping our SaaS. This piggybacks off the beacon option and adds an API endpoint to allow the frontend to potentially track metrics back to our SaaS beacon endpoint. We will use this to figure out the number of installs that are using APIs that we would like to deprecate. See #25744 for a discussion around this.
5b78211
to
5e5f132
Compare
ee4f9d9
to
7bafdaa
Compare
…tries async (#25744) Follow up to #25566 (and the revert #25681) - there was an issue where `jQuery` (as well as `SentryApp` global) is no longer available before the `DOMContentLoaded` event because our js bundles are loaded async with this PR. See #25731, #25728 for fixes ## Breaking changes See [this forum post](https://forum.sentry.io/t/breaking-frontend-changes-for-custom-plugins/14184) for more information, but this has breaking changes for custom plugins (if you are hosting Sentry yourself). ### Original PR description This changes our entrypoints to dynamically import the main bundles (i.e. the previous entrypoints). The goal here is to make the entrypoints as minimal as possible as we may need these to be uncached for splitting off frontend deploys. The other change is removing the vendor chunk that we had, and instead use webpack defaults and let it decide how to chunk our build. This results in the client needing to request more + smaller chunks (instead of fewer + larger chunks). This costs a bit of network overhead, but a trade-off here is that the smaller chunks should have longer cache lives. I'd like to push this out and evaluate how it affects our web vitals in prod and then make a decision on how we'd like to proceed. Here's a light benchmarking of the perf changes, I used Chrome's dev tools to throttle a mix of the CPU and network bandwidth: (tbh `DOMContentLoaded` doesn't seem like a very practical measurement here, but it's what was readily available in CDT) ![image](https://user-images.githubusercontent.com/79684/116126971-1e556300-a67c-11eb-948e-69e07ac3078b.png) (private spreadsheet: https://docs.google.com/spreadsheets/d/1EN2jD0fqAJCASaAL1hAEBZJScTutmCZgCvDooamB3u4/) ### Locales We were also previously loading locales for moment, but never setting the locale with `moment.locale()`, this PR addresses this as well as simplifying the webpack logic around locales (and instead pushing them into the application code).
This builds on top of #25744 where we refactored our JS entry points to be lightweight and to use dynamic imports (via webpack) to load the rest of the JS application. This PR will now remove the need for the backend to be updated in order to release a new frontend. This will allow us to have frontend-only deploys which should complete much faster than a full deploy. We accomplish this by removing the webpack manifest plugin introduced in #25234, where we previously were versioning by a release string in the asset path. These unversioned asset files will be served using a [`Cache-Control: no-cache`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#cacheability) header which tells clients that they need to check against the origin server to see if the asset has changed, if it has not, they can use their cached copy. These unversioned files will end up changing quite frequently with every release, but they should be relatively small. They contain the webpack runtime that allows us to load the rest of our application bundles, which are versioned by a hash of their contents. These versioned chunks will have a long-term `max-age` for caching as the hash in the file name will serve to cache bust.
This fixes a regression introduced in #25744 - where the users language option is now being ignored.
This fixes a regression introduced in #25744 - where the users language option is now being ignored.
This reverts commit 3228492. This builds on top of #25744 where we refactored our JS entry points to be lightweight and to use dynamic imports (via webpack) to load the rest of the JS application. This PR will now remove the need for the backend to be updated in order to release a new frontend. This will allow us to have frontend-only deploys which should complete much faster than a full deploy. We accomplish this by removing the webpack manifest plugin introduced in #25234, where we previously were versioning by a release string in the asset path. These unversioned asset files will be served using a [`Cache-Control: no-cache`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#cacheability) header which tells clients that they need to check against the origin server to see if the asset has changed, if it has not, they can use their cached copy. These unversioned files will end up changing quite frequently with every release, but they should be relatively small. They contain the webpack runtime that allows us to load the rest of our application bundles, which are versioned by a hash of their contents. These versioned chunks will have a long-term `max-age` for caching as the hash in the file name will serve to cache bust. Note: this was reverted in #25994 as it broke production due to our CDN having cached an old asset due to a cache misconfiguration during development. This was also not caught in staging because we needed to change the asset paths for staging deploys to avoid clobbering production assets.
This reverts commit 3228492. This builds on top of #25744 where we refactored our JS entry points to be lightweight and to use dynamic imports (via webpack) to load the rest of the JS application. This PR will now remove the need for the backend to be updated in order to release a new frontend. This will allow us to have frontend-only deploys which should complete much faster than a full deploy. We accomplish this by removing the webpack manifest plugin introduced in #25234, where we previously were versioning by a release string in the asset path. These unversioned asset files will be served using a [`Cache-Control: no-cache`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#cacheability) header which tells clients that they need to check against the origin server to see if the asset has changed, if it has not, they can use their cached copy. These unversioned files will end up changing quite frequently with every release, but they should be relatively small. They contain the webpack runtime that allows us to load the rest of our application bundles, which are versioned by a hash of their contents. These versioned chunks will have a long-term `max-age` for caching as the hash in the file name will serve to cache bust. Note: this was reverted in #25994 as it broke production due to our CDN having cached an old asset due to a cache misconfiguration during development. This was also not caught in staging because we needed to change the asset paths for staging deploys to avoid clobbering production assets.
This reverts commit 388edc8. This builds on top of #25744 where we refactored our JS entry points to be lightweight and to use dynamic imports (via webpack) to load the rest of the JS application. This PR will now remove the need for the backend to be updated in order to release a new frontend. This will allow us to have frontend-only deploys which should complete much faster than a full deploy. We accomplish this by removing the webpack manifest plugin introduced in #25234, where we previously were versioning b y a release string in the asset path. These unversioned asset files will be served using a [`Cache-Control: no-cache`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache -Control#cacheability) header which tells clients that they need to check against the origi n server to see if the asset has changed, if it has not, they can use their cached copy. Th ese unversioned files will end up changing quite frequently with every release, but they sh ould be relatively small. They contain the webpack runtime that allows us to load the rest of our application bundles, which are versioned by a hash of their contents. These versione d chunks will have a long-term `max-age` for caching as the hash in the file name will serv e to cache bust. Note: this was reverted in #25994 as it broke p roduction due to our CDN having cached an old asset due to a cache misconfiguration during development. This was also not caught in staging because we needed to change the asset path s for staging deploys to avoid clobbering production assets, and the staging path was not p reviously cached. Requires #26987
Follow up to #25566 (and the revert #25681) - there was an issue where
jQuery
(as well asSentryApp
global) is no longer available before theDOMContentLoaded
event because our js bundles are loaded async with this PR. See #25731, #25728 for fixesBreaking changes
See this forum post for more information, but this has breaking changes for custom plugins (if you are hosting Sentry yourself).
Original PR description
This changes our entrypoints to dynamically import the main bundles (i.e. the previous entrypoints). The goal here is to make the entrypoints as minimal as possible as we may need these to be uncached for splitting off frontend deploys.
The other change is removing the vendor chunk that we had, and instead use webpack defaults and let it decide how to chunk our build. This results in the client needing to request more + smaller chunks (instead of fewer + larger chunks). This costs a bit of network overhead, but a trade-off here is that the smaller chunks should have longer cache lives.
I'd like to push this out and evaluate how it affects our web vitals in prod and then make a decision on how we'd like to proceed.
Here's a light benchmarking of the perf changes, I used Chrome's dev tools to throttle a mix of the CPU and network bandwidth:
(tbh
DOMContentLoaded
doesn't seem like a very practical measurement here, but it's what was readily available in CDT)(private spreadsheet: https://docs.google.com/spreadsheets/d/1EN2jD0fqAJCASaAL1hAEBZJScTutmCZgCvDooamB3u4/)
Locales
We were also previously loading locales for moment, but never setting the locale with
moment.locale()
, this PR addresses this as well as simplifying the webpack logic around locales (and instead pushing them into the application code).