Skip to content
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

Merged

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Apr 28, 2021

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 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

(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).

@billyvg
Copy link
Member Author

billyvg commented Apr 28, 2021

@BYK there's a chance this could affect custom plugins (this will break the assumption that jQuery and window.SentryApp is loaded synchronously and will be available to their scripts), but not quite sure how to check, any thoughts?

@billyvg
Copy link
Member Author

billyvg commented Apr 29, 2021

U2f:
image
SetupWizard:
image
Sudo:
image

.eslintrc.js Outdated Show resolved Hide resolved
Comment on lines 49 to 54
<div id="u2f-challenge">
<div class="loading">
<div class="loading-mask"></div>
<div class="loading-indicator"></div>
</div>
</div>
Copy link
Member Author

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

Comment on lines 59 to 60
name: 'renderReact',
component: 'U2fSign',
Copy link
Member Author

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

@billyvg billyvg marked this pull request as ready for review April 30, 2021 17:11
@billyvg billyvg requested a review from a team as a code owner April 30, 2021 17:11
@billyvg billyvg requested review from a team and removed request for a team April 30, 2021 17:11
@billyvg
Copy link
Member Author

billyvg commented Apr 30, 2021

@BYK
Copy link
Member

BYK commented Apr 30, 2021

@billyvg how would a plugin make use of jQuery and window.SentryApp after this change? As long as I know, unless they are bundled together with the whole app, there's no way for them to require or import these modules or somehow wait for them to become available?

@billyvg
Copy link
Member Author

billyvg commented Apr 30, 2021

@BYK For jQuery, I would recommend including the package in the plugin template like we did for create_issue https://github.com/getsentry/sentry/pull/25744/files#diff-c86d6e5e14407f0eb6a3b56631e435a469e94569b382f085ff22bc9ffea94267R15-R16 - the main reason for this is because we're aiming to remove jQuery from the frontend completely at some point in the future.

For window.SentryApp - we can add a callback for them to use when the bundle is loaded + application has rendered to dom. It'd be nice to find out who is using what, as I'd like to have it documented somewhere so that these external dependencies don't somehow disappear in the future (if they are being used).

How does that sound?

@BYK
Copy link
Member

BYK commented May 3, 2021

the main reason for this is because we're aiming to remove jQuery from the frontend completely at some point in the future.

YESSSS!!!! 🥳

For jQuery, I would recommend including the package in the plugin template like we did for create_issue

This works for me.

For window.SentryApp - we can add a callback for them to use when the bundle is loaded + application has rendered to dom. It'd be nice to find out who is using what, as I'd like to have it documented somewhere so that these external dependencies don't somehow disappear in the future (if they are being used).

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.

@billyvg
Copy link
Member Author

billyvg commented May 3, 2021

I think the key thing here is, we cannot just do this overnight.

@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.

@billyvg billyvg removed request for a team May 3, 2021 17:07
@billyvg billyvg marked this pull request as draft May 3, 2021 17:07
@BYK
Copy link
Member

BYK commented May 3, 2021

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?

@billyvg
Copy link
Member Author

billyvg commented May 3, 2021

@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?

@BYK
Copy link
Member

BYK commented May 3, 2021

How would we handle the installs that don't enable the beacon?

We won't :)

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

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.

@billyvg
Copy link
Member Author

billyvg commented May 3, 2021

We could do it direct from frontend, but we would need to expose the beacon configuration to the user/frontend.

billyvg added a commit that referenced this pull request May 4, 2021
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.
@BYK
Copy link
Member

BYK commented May 5, 2021

but we would need to expose the beacon configuration to the user/frontend.

Not sure how hard that would be. I'll leave this to you.

billyvg added a commit that referenced this pull request May 6, 2021
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.
@billyvg billyvg force-pushed the build/webpack/refactor-to-load-vendor-from-app-entrypoint-try-2 branch from 5b78211 to 5e5f132 Compare May 10, 2021 18:30
@billyvg billyvg force-pushed the build/webpack/refactor-to-load-vendor-from-app-entrypoint-try-2 branch from ee4f9d9 to 7bafdaa Compare June 23, 2021 21:30
@billyvg billyvg merged commit 07c0cc4 into master Jun 23, 2021
@billyvg billyvg deleted the build/webpack/refactor-to-load-vendor-from-app-entrypoint-try-2 branch June 23, 2021 21:56
maxiuyuan pushed a commit that referenced this pull request Jun 24, 2021
…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).
billyvg added a commit that referenced this pull request Jun 29, 2021
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.
billyvg added a commit that referenced this pull request Jun 30, 2021
This fixes a regression introduced in #25744 - where the users language option is now being ignored.
billyvg added a commit that referenced this pull request Jun 30, 2021
This fixes a regression introduced in #25744 - where the users language option is now being ignored.
billyvg added a commit that referenced this pull request Jul 1, 2021
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.
billyvg added a commit that referenced this pull request Jul 1, 2021
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.
billyvg added a commit that referenced this pull request Jul 7, 2021
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
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants