Skip to content

Use relative path resolution and manually include static assets in vite's module graph. #23209

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

Merged
merged 4 commits into from
Mar 25, 2025

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Mar 24, 2025

Fixes: mozilla/addons#15486

Description

  • Use relative paths for production URLs
  • include static assets (png|svg) manually in vite

Context

The path where we serve static assets impacts several parts of the codebase:

  • the nginx path we route requests to static asset servers
  • settings which exempt static urls from having a lang/app prefix
  • settings which are used to produce static urls in html
  • settings in vite itself to determine how to transform URLs during dev/build

Since all of these have to be syncronized it makes sense to extract the part of the URL which defines the "root" or "prefix" of the url to its own environment variable setting.

By default, the values in base_settings.py will be the same, and these values are explicitly overriden in all production like environments. But in local environments this allows one to configure the path where static files are served from and linked to from a single place.

Testing

clean project

make down \
  && rm -rf ./static-build ./site-static \
  && git clean -xdf ./static \
  && echo "finished!"

Dev mode

make up DOCKER_TARGET=development

Navigate the site, ensuring that static images, css files and js files are resolving correctly.

  • /developers*
  • /reviewers*

There are 141 URL imports in our css files, spread across virtually every page. It should be easy to find non-resolving URLs but make sure to be thorough and follow all links across devhub/reviewer tools

Prod mode

make down && make up DOCKER_TARGET=production

Expect the same as in dev mode. URLs should resolve correctly, though the css files will be bundled and minified.

Build

Verify the build is resolving correctly

make update_assets

Now inspect ./site-static/assets/css, find a css file that imports files devhub.css for example (note: it will have a hash postfix). Check for refrences to url( and expect all references should use relative paths pointing to ../<path>. This path should match a file existing at the expected path (in the parent).

Note

Because vite transforms the file paths, nested files are collapsed so any file in source will point to ../<name>.<ext> in the build. All css file as in <static>/css and all images are in <static>/<name>.<ext> so all css files will point to the parent directory when referencing static images.

Dynamic STATIC_URL

Re-run the development build setting a value for the STATIC_URL_PREFIX

make up STATIC_URL_PREFIX=/static-server/

Important

Make sure to include the / trailing slash.

This will modify the path we expect to serve static files from for nginx, web and static (vite)

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind requested a review from diox March 24, 2025 10:44
@KevinMind KevinMind marked this pull request as draft March 24, 2025 11:05
@KevinMind KevinMind force-pushed the kevinmind/addons/15486 branch 3 times, most recently from 26048f9 to b013dbf Compare March 25, 2025 09:37
@KevinMind KevinMind force-pushed the kevinmind/addons/15486 branch from b013dbf to 0f246a9 Compare March 25, 2025 09:40
@KevinMind KevinMind marked this pull request as ready for review March 25, 2025 09:40
@KevinMind KevinMind requested a review from diox March 25, 2025 09:40
@diox
Copy link
Member

diox commented Mar 25, 2025

In development mode, in devhub when looking at one of my add-ons, I got an error in the devtools console I've never seen before:

jQuery.Deferred exception: $.cookie is not a function @http://olympia.test/static-server/bundle/js/zamboni/l10n.js:181:20
mightThrow@http://olympia.test/static-server/bundle/@fs/data/olympia/node_modules/.vite/deps/chunk-GQWXXZXP.js?v=775b3856:2010:40
node_modules/jquery/dist/jquery.js/</Deferred/then/resolve/</process<@http://olympia.test/static-server/bundle/@fs/data/olympia/node_modules/.vite/deps/chunk-GQWXXZXP.js?v=775b3856:2048:23
 undefined

I repeated the entire clean process, went to master branch, couldn't reproduce, went back to this branch and cleaned again, and couldn't reproduce either. But this might be worth investigating...

@KevinMind
Copy link
Contributor Author

In development mode, in devhub when looking at one of my add-ons, I got an error in the devtools console I've never seen before:

jQuery.Deferred exception: $.cookie is not a function @http://olympia.test/static-server/bundle/js/zamboni/l10n.js:181:20
mightThrow@http://olympia.test/static-server/bundle/@fs/data/olympia/node_modules/.vite/deps/chunk-GQWXXZXP.js?v=775b3856:2010:40
node_modules/jquery/dist/jquery.js/</Deferred/then/resolve/</process<@http://olympia.test/static-server/bundle/@fs/data/olympia/node_modules/.vite/deps/chunk-GQWXXZXP.js?v=775b3856:2048:23
 undefined

I repeated the entire clean process, went to master branch, couldn't reproduce, went back to this branch and cleaned again, and couldn't reproduce either. But this might be worth investigating...

Could you share here or on slack the contents of that file when you open the URL in the browser? It could be that the jquery global doesn't exist by the time we define $.cookie, but we have it injected in all bundles so would be odd.

@diox
Copy link
Member

diox commented Mar 25, 2025

https://gist.github.com/diox/301acfb19f462e8e51bdebcf35ef5ba8 but my initial reaction was to switch to master and back again so I don't know if that's the contents I had when I saw that error. I haven't been able to see it since, it's possible it was an old warning from before I switched to your branch in the first place.

@KevinMind
Copy link
Contributor Author

KevinMind commented Mar 25, 2025

https://gist.github.com/diox/301acfb19f462e8e51bdebcf35ef5ba8 but my initial reaction was to switch to master and back again so I don't know if that's the contents I had when I saw that error. I haven't been able to see it since, it's possible it was an old warning from before I switched to your branch in the first place.

So it is including the jquery import and even hoisting correctly.

However, I have noticed there can be some odd behavior around the jquery global. This could (potentially) be a race condition in the loading order of scripts. I don't have enough info to confirm that but we could consider isolating the jquery initialization script to it's own module, and (like in init.js) explicitly set it to a global. Throw that on the top of <head> and call it a day.

I'm leaning more towards a stale file being served due to one or another caching issue.

With the number of changes it is unlikely there are zero bugs here, so we should probably have a ticket for that in case it happens again and then investigate further.

mozilla/addons#15493

@KevinMind KevinMind merged commit 963616e into master Mar 25, 2025
42 checks passed
@KevinMind KevinMind deleted the kevinmind/addons/15486 branch March 25, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Static assets not served correctly
2 participants