Browse Source

build: Use `django-manifest-loader` for static assets (#25234)

This changes the way we handle static assets. Previously, the assets were versioned by either a `version` file (commit sha?) or a timestamp. This changes it so that *only* assets generated from `webpack` are served at `/_assets/` and include a hash (based on file contents) in their filename. `webpack` also produces a manifest.json file that maps a key -> hashed filename which `django-manifest-loader` uses to load static assets.

This will allow us to have better long term caching on these `webpack` assets as they will be cached until the contents of their chunks changes. Contrast that to the existing behavior where all assets are cached by the current release.

Assets that are not processed by `webpack` (generally static assets only referenced from django templates) are still served at the existing URL. In the future we may have these processed through `webpack` so they can also be cached beyond the current release. However, the reason we'll want to keep this other static URL is for django apps like `django.contrib.admin` so that they can be at least cached by version. Alternatively, we could also choose to not cache these at all. AFAICT it's only the admin where this is needed, so it won't be used that often.

Old:

```
/_static/{version}/sentry/dist/app.js
```

New:
```
/_assets/sentry/app.{appHash}.js
```

* ~Check CDN (fastly) settings to make sure that non-webpack assets are not cached long term~ No longer applies as we will maintain the existing URL versioning for non-webpack assets.
Billy Vong 3 years ago
parent
commit
5767d2ba89

+ 5 - 1
.github/workflows/acceptance.yml

@@ -64,11 +64,15 @@ jobs:
           name: jest-html
           path: .artifacts/visual-snapshots/jest
 
+      - name: Get CSS filename
+        id: css-manifest
+        run: echo "::set-output name=sentrycss::$(jq -r '.["sentry.css"]' src/sentry/static/sentry/dist/manifest.json)"
+
       - name: Create Images from HTML
         uses: getsentry/action-html-to-image@main
         with:
           base-path: .artifacts/visual-snapshots/jest
-          css-path: src/sentry/static/sentry/dist/sentry.css
+          css-path: src/sentry/static/sentry/dist/${{ steps.css-manifest.outputs.sentrycss }}
 
       - name: Save snapshots
         if: always()

+ 25 - 0
conftest.py

@@ -1,6 +1,10 @@
 import os
 import sys
 
+dist_path = os.path.abspath(
+    os.path.join(os.path.dirname(__file__), "src", "sentry", "static", "sentry", "dist")
+)
+manifest_path = os.path.join(dist_path, "manifest.json")
 pytest_plugins = ["sentry.utils.pytest"]
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), "src"))
@@ -12,3 +16,24 @@ def pytest_configure(config):
     # XXX(dcramer): Kombu throws a warning due to transaction.commit_manually
     # being used
     warnings.filterwarnings("error", "", Warning, r"^(?!(|kombu|raven|sentry))")
+
+    # Create an empty webpack manifest file - otherwise tests will crash if it does not exist
+    os.makedirs(dist_path, exist_ok=True)
+
+    # Only create manifest if it doesn't exist
+    # (e.g. acceptance tests will have an actual manifest from webpack)
+    if os.path.exists(manifest_path):
+        return
+
+    with open(manifest_path, "w+") as fp:
+        fp.write("{}")
+
+
+def pytest_unconfigure():
+    if not os.path.exists(manifest_path):
+        return
+
+    # Clean up manifest file if contents are empty
+    with open(manifest_path) as f:
+        if f.read() == "{}":
+            os.remove(manifest_path)

+ 1 - 0
package.json

@@ -133,6 +133,7 @@
     "webpack": "5.27.2",
     "webpack-cli": "4.5.0",
     "webpack-remove-empty-scripts": "^0.7.1",
+    "webpack-manifest-plugin": "^3.1.0",
     "wink-jaro-distance": "^2.0.0",
     "zxcvbn": "^4.4.2"
   },

+ 1 - 0
requirements-base.txt

@@ -8,6 +8,7 @@ confluent-kafka==1.6.0; python_version == '3.9'
 croniter==0.3.34
 datadog==0.29.3
 django-crispy-forms==1.7.2
+django-manifest-loader==1.0.0
 django-picklefield==1.0.0
 django-sudo==3.1.0
 Django==1.11.29

+ 1 - 1
scripts/lib.sh

@@ -188,7 +188,7 @@ bootstrap() {
 
 clean() {
     echo "--> Cleaning static cache"
-    rm -rf dist/* static/dist/*
+    rm -rf dist/* src/sentry/static/sentry/dist/*
     echo "--> Cleaning integration docs cache"
     rm -rf src/sentry/integration-docs
     echo "--> Cleaning pyc files"

+ 12 - 0
src/sentry/conf/server.py

@@ -330,6 +330,7 @@ INSTALLED_APPS = (
     "django.contrib.sites",
     "crispy_forms",
     "rest_framework",
+    "manifest_loader",
     "sentry",
     "sentry.analytics",
     "sentry.incidents.apps.Config",
@@ -367,6 +368,17 @@ SILENCED_SYSTEM_CHECKS = (
 
 STATIC_ROOT = os.path.realpath(os.path.join(PROJECT_ROOT, "static"))
 STATIC_URL = "/_static/{version}/"
+# webpack assets live at a different URL that is unversioned
+# as we configure webpack to include file content based hash in the filename
+STATIC_MANIFEST_URL = "/_static/dist/"
+
+# The webpack output directory, used by django-manifest-loader
+STATICFILES_DIRS = [
+    os.path.join(STATIC_ROOT, "sentry", "dist"),
+]
+
+# django-manifest-loader settings
+MANIFEST_LOADER = {"cache": True}
 
 # various middleware will use this to identify resources which should not access
 # cookies

+ 1 - 1
src/sentry/constants.py

@@ -74,7 +74,7 @@ SENTRY_APP_SLUG_MAX_LENGTH = 64
 MAX_ROLLUP_POINTS = 10000
 
 
-# Team slugs which may not be used. Generally these are top level URL patterns
+# Organization slugs which may not be used. Generally these are top level URL patterns
 # which we don't want to worry about conflicts on.
 RESERVED_ORGANIZATION_SLUGS = frozenset(
     (

+ 1 - 1
src/sentry/templates/sentry/base-react.html

@@ -9,7 +9,7 @@
       <div class="loading-mask"></div>
 
       <div class="loading-indicator">
-        <img src="{% asset_url 'sentry' 'images/sentry-loader.svg' %}" />
+        <img src="{% manifest_asset_url 'sentry' 'images/sentry-loader.svg' %}" />
       </div>
 
       <div class="loading-message">

+ 1 - 1
src/sentry/templates/sentry/bases/react_pipeline.html

@@ -14,6 +14,6 @@
 {% endblock %}
 
 {% block scripts_main_entrypoint %}
-    {% asset_url "sentry" "dist/pipeline.js" as asset_url %}
+    {% manifest_asset_url "sentry" "pipeline.js" as asset_url %}
     {% script src=asset_url data-entry="true" %}{% endscript %}
 {% endblock %}

+ 7 - 3
src/sentry/templates/sentry/layout.html

@@ -25,7 +25,7 @@
 
   <link rel="mask-icon" sizes="any" href="{% absolute_asset_url "sentry" "images/logos/logo-sentry.svg" %}" color="#FB4226">
 
-  <link href="{% asset_url "sentry" "dist/sentry.css" %}" rel="stylesheet"/>
+  <link href="{% manifest_asset_url "sentry" "sentry.css" %}" rel="stylesheet"/>
 
   {% block css %}{% endblock %}
 
@@ -45,11 +45,15 @@
 
   {% block scripts %}
   {% locale_js_include %}
-  {% asset_url "sentry" "dist/vendor.js" as asset_url %}
+
+  {% manifest_asset_url "sentry" "runtime.js" as asset_url %}
+  {% script src=asset_url %}{% endscript %}
+
+  {% manifest_asset_url "sentry" "vendor.js" as asset_url %}
   {% script src=asset_url %}{% endscript %}
 
   {% block scripts_main_entrypoint %}
-    {% asset_url "sentry" "dist/app.js" as asset_url %}
+    {% manifest_asset_url "sentry" "app.js" as asset_url %}
     {% script src=asset_url data-entry="true" %}{% endscript %}
   {% endblock %}
 

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