Browse Source

build(webpack): Change webpack to have smaller chunks and make all entries 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 https://github.com/getsentry/sentry/pull/25731, https://github.com/getsentry/sentry/pull/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).
Billy Vong 3 years ago
parent
commit
07c0cc4aec

+ 0 - 40
build-utils/optional-locale-chunk-plugin.js

@@ -1,40 +0,0 @@
-/* eslint-env node */
-/* eslint import/no-nodejs-modules:0 */
-
-/**
- * When our locales are codesplit into cache groups, webpack expects that all
- * chunks *must* be loaded before the main entrypoint can be executed. However,
- * since we will only be using one locale at a time we do not want to load all
- * locale chunks, just the one the user has enabled.
- *
- * This plugin removes the locale chunks from the app entrypoint's immediate
- * chunk dependants list, ensuring that the compiled entrypoint will execute
- * *without* all locale chunks loaded.
- *
- * Note that the locale actually gets initially loaded in the server template and
- * the frontend code (in `app/translations`) may be redundant.
- */
-const PLUGIN_NAME = 'OptionalLocaleChunkPlugin';
-
-const clearLocaleChunks = chunks => {
-  Array.from(chunks)
-    .filter(chunk => chunk.name !== 'app')
-    .forEach(chunk => {
-      const mainGroup = Array.from(chunk.groupsIterable)[0];
-      // With the upgrade from webpack4 -> 5, we have a lot more chunks without a name
-      // Not entirely sure why, but we want to keep these chunks without names
-      mainGroup.chunks = mainGroup.chunks.filter(
-        c => !c.name || !c.name.startsWith('locale')
-      );
-    });
-};
-
-class OptionalLocaleChunkPlugin {
-  apply(compiler) {
-    compiler.hooks.compilation.tap(PLUGIN_NAME, compilation =>
-      compilation.hooks.afterOptimizeChunks.tap(PLUGIN_NAME, clearLocaleChunks)
-    );
-  }
-}
-
-module.exports = OptionalLocaleChunkPlugin;

+ 1 - 0
config/tsconfig.build.json

@@ -36,6 +36,7 @@
       "app/*": ["static/app/*"],
       "app/*": ["static/app/*"],
       "sentry-test/*": ["tests/js/sentry-test/*"],
       "sentry-test/*": ["tests/js/sentry-test/*"],
       "sentry-images/*": ["static/images/*"],
       "sentry-images/*": ["static/images/*"],
+      "sentry-locale/*": ["src/sentry/locale/*"],
       "sentry-logos/*": ["src/sentry/static/sentry/images/logos/*"],
       "sentry-logos/*": ["src/sentry/static/sentry/images/logos/*"],
       "sentry-fonts/*": ["static/fonts/*"],
       "sentry-fonts/*": ["static/fonts/*"],
       // Use the stub file for typechecking. Webpack resolver will use the real files
       // Use the stub file for typechecking. Webpack resolver will use the real files

+ 1 - 0
jest.config.js

@@ -41,6 +41,7 @@ module.exports = {
   snapshotSerializers: ['enzyme-to-json/serializer'],
   snapshotSerializers: ['enzyme-to-json/serializer'],
   moduleNameMapper: {
   moduleNameMapper: {
     '^sentry-test/(.*)': '<rootDir>/tests/js/sentry-test/$1',
     '^sentry-test/(.*)': '<rootDir>/tests/js/sentry-test/$1',
+    '^sentry-locale/(.*)': '<rootDir>/src/sentry/locale/$1',
     '\\.(css|less|png|jpg|mp4)$': '<rootDir>/tests/js/sentry-test/importStyleMock.js',
     '\\.(css|less|png|jpg|mp4)$': '<rootDir>/tests/js/sentry-test/importStyleMock.js',
     '\\.(svg)$': '<rootDir>/tests/js/sentry-test/svgMock.js',
     '\\.(svg)$': '<rootDir>/tests/js/sentry-test/svgMock.js',
     'integration-docs-platforms':
     'integration-docs-platforms':

+ 0 - 5
src/sentry/templates/sentry/layout.html

@@ -53,14 +53,9 @@
   {% endscript %}
   {% endscript %}
 
 
   {% block scripts %}
   {% block scripts %}
-  {% locale_js_include %}
-
   {% manifest_asset_url "sentry" "runtime.js" as asset_url %}
   {% manifest_asset_url "sentry" "runtime.js" as asset_url %}
   {% script src=asset_url %}{% endscript %}
   {% script src=asset_url %}{% endscript %}
 
 
-  {% manifest_asset_url "sentry" "vendor.js" as asset_url %}
-  {% script src=asset_url %}{% endscript %}
-
   {% block scripts_main_entrypoint %}
   {% block scripts_main_entrypoint %}
     {% manifest_asset_url "sentry" "app.js" as asset_url %}
     {% manifest_asset_url "sentry" "app.js" as asset_url %}
     {% script src=asset_url data-entry="true" %}{% endscript %}
     {% script src=asset_url data-entry="true" %}{% endscript %}

+ 2 - 0
src/sentry/templates/sentry/plugins/bases/issue/create_issue.html

@@ -12,6 +12,8 @@
 
 
 {% block scripts %}
 {% block scripts %}
   {{ block.super }}
   {{ block.super }}
+  {% asset_url 'sentry' 'vendor/jquery.2.2.4.min.js' as jquery_url %}
+  {% script src=jquery_url %}{% endscript %}
   {% asset_url "sentry" "vendor/select2.min.js" as asset_url %}
   {% asset_url "sentry" "vendor/select2.min.js" as asset_url %}
   {% script src=asset_url %}{% endscript %}
   {% script src=asset_url %}{% endscript %}
   {% asset_url "sentry" "vendor/tooltip.min.js" as asset_url %}
   {% asset_url "sentry" "vendor/tooltip.min.js" as asset_url %}

+ 0 - 28
src/sentry/templatetags/sentry_assets.py

@@ -3,7 +3,6 @@ import re
 from django import template
 from django import template
 from django.conf import settings
 from django.conf import settings
 from django.template.base import token_kwargs
 from django.template.base import token_kwargs
-from django.utils.safestring import mark_safe
 
 
 from sentry import options
 from sentry import options
 from sentry.utils.assets import get_asset_url, get_manifest_url
 from sentry.utils.assets import get_asset_url, get_manifest_url
@@ -39,33 +38,6 @@ def crossorigin():
     return ' crossorigin="anonymous"'
     return ' crossorigin="anonymous"'
 
 
 
 
-@register.simple_tag(takes_context=True)
-def locale_js_include(context):
-    """
-    If the user has a non-English locale set, returns a <script> tag pointing
-    to the relevant locale JavaScript file
-    """
-    request = context["request"]
-
-    try:
-        lang_code = request.LANGUAGE_CODE
-    except AttributeError:
-        # it's possible that request at this point, LANGUAGE_CODE hasn't be bound
-        # to the Request object yet. This specifically happens when rendering our own
-        # 500 error page, resulting in yet another error trying to render our error.
-        return ""
-
-    if lang_code == "en" or lang_code not in settings.SUPPORTED_LANGUAGES:
-        return ""
-
-    nonce = ""
-    if hasattr(request, "csp_nonce"):
-        nonce = f' nonce="{request.csp_nonce}"'
-
-    href = get_manifest_url("sentry", "locale/" + lang_code + ".js")
-    return mark_safe(f'<script src="{href}"{crossorigin()}{nonce}></script>')
-
-
 @register.tag
 @register.tag
 def script(parser, token):
 def script(parser, token):
     """
     """

+ 0 - 9
static/app/__mocks__/translations.tsx

@@ -1,9 +0,0 @@
-export function getTranslations() {
-  return {
-    '': {
-      domain: 'the_domain',
-      lang: 'en',
-      plural_forms: 'nplurals=2; plural=(n != 1);',
-    },
-  };
-}

+ 25 - 0
static/app/bootstrap/initializeApp.tsx

@@ -0,0 +1,25 @@
+import 'bootstrap/js/alert';
+import 'bootstrap/js/tab';
+import 'bootstrap/js/dropdown';
+import './exportGlobals';
+
+import routes from 'app/routes';
+import {Config} from 'app/types';
+import {metric} from 'app/utils/analytics';
+
+import {commonInitialization} from './commonInitialization';
+import {initializeSdk} from './initializeSdk';
+import {processInitQueue} from './processInitQueue';
+import {renderMain} from './renderMain';
+import {renderOnDomReady} from './renderOnDomReady';
+
+export async function initializeApp(config: Config) {
+  commonInitialization(config);
+  initializeSdk(config, {routes});
+
+  // Used for operational metrics to determine that the application js
+  // bundle was loaded by browser.
+  metric.mark({name: 'sentry-app-init'});
+  renderOnDomReady(renderMain);
+  processInitQueue();
+}

+ 78 - 0
static/app/bootstrap/initializeLocale.tsx

@@ -0,0 +1,78 @@
+import * as Sentry from '@sentry/react';
+import * as moment from 'moment';
+import * as qs from 'query-string';
+
+import {DEFAULT_LOCALE_DATA, setLocale} from 'app/locale';
+import {Config} from 'app/types';
+
+// zh-cn => zh_CN
+function convertToDjangoLocaleFormat(language: string) {
+  const [left, right] = language.split('-');
+  return left + (right ? '_' + right.toUpperCase() : '');
+}
+
+async function getTranslations(language: string) {
+  language = convertToDjangoLocaleFormat(language);
+
+  // No need to load the english locale
+  if (language === 'en') {
+    return DEFAULT_LOCALE_DATA;
+  }
+
+  try {
+    return await import(`sentry-locale/${language}/LC_MESSAGES/django.po`);
+  } catch (e) {
+    Sentry.withScope(scope => {
+      scope.setLevel(Sentry.Severity.Warning);
+      scope.setFingerprint(['sentry-locale-not-found']);
+      scope.setExtra('locale', language);
+      Sentry.captureException(e);
+    });
+
+    // Default locale if not found
+    return DEFAULT_LOCALE_DATA;
+  }
+}
+
+/**
+ * Initialize locale
+ *
+ * This *needs* to be initialized as early as possible (e.g. before `app/locale` is used),
+ * otherwise the rest of the application will fail to load.
+ *
+ * Priority:
+ *
+ * - URL params (`?lang=en`)
+ * - User configuration options
+ * - "en" as default
+ */
+export async function initializeLocale(config: Config) {
+  let queryString: qs.ParsedQuery = {};
+
+  // Parse query string for `lang`
+  try {
+    queryString = qs.parse(window.location.search) || {};
+  } catch {
+    // ignore if this fails to parse
+    // this can happen if we have an invalid query string
+    // e.g. unencoded "%"
+  }
+
+  const queryStringLang = Array.isArray(queryString.lang)
+    ? queryString.lang[0]
+    : queryString.lang;
+  const languageCode = queryStringLang || config.languageCode || 'en';
+
+  try {
+    const translations = await getTranslations(languageCode);
+    setLocale(translations);
+
+    // No need to import english
+    if (languageCode !== 'en') {
+      await import(`moment/locale/${languageCode}`);
+      moment.locale(languageCode);
+    }
+  } catch (err) {
+    Sentry.captureException(err);
+  }
+}

+ 11 - 20
static/app/bootstrap/initializeMain.tsx

@@ -1,25 +1,16 @@
-import 'bootstrap/js/alert';
-import 'bootstrap/js/tab';
-import 'bootstrap/js/dropdown';
-import './exportGlobals';
-
-import routes from 'app/routes';
 import {Config} from 'app/types';
 import {Config} from 'app/types';
-import {metric} from 'app/utils/analytics';
 
 
-import {commonInitialization} from './commonInitialization';
-import {initializeSdk} from './initializeSdk';
-import {processInitQueue} from './processInitQueue';
-import {renderMain} from './renderMain';
-import {renderOnDomReady} from './renderOnDomReady';
+import {initializeLocale} from './initializeLocale';
 
 
-export function initializeMain(config: Config) {
-  commonInitialization(config);
-  initializeSdk(config, {routes});
+export async function initializeMain(config: Config) {
+  // This needs to be loaded as early as possible, or else the locale library can
+  // throw an exception and prevent the application from being loaded.
+  //
+  // e.g. `app/constants` uses `t()` and is imported quite early
+  await initializeLocale(config);
 
 
-  // Used for operational metrics to determine that the application js
-  // bundle was loaded by browser.
-  metric.mark({name: 'sentry-app-init'});
-  renderOnDomReady(renderMain);
-  processInitQueue();
+  // This is dynamically imported because we need to make sure locale is configured
+  // before proceeding.
+  const {initializeApp} = await import('./initializeApp');
+  await initializeApp(config);
 }
 }

Some files were not shown because too many files changed in this diff