Skip to content

Migrate remaining files to vite #23180

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 2 commits into from
Mar 20, 2025
Merged

Migrate remaining files to vite #23180

merged 2 commits into from
Mar 20, 2025

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Mar 17, 2025

Fixes: mozilla/addons#15

Context

The remaining pages are very tightly dependant on specific shared modules so it is easiest to migrate them all at once.

After this we can remove the non vite compilation logic and add stricter linting to remove dead code and enforce all the styling rules.

Testing

For each of these pages:

  • navigate to the page
  • verify there are no failing css/js files
  • verify there are no unexpected console.log messages (exclude cors about google analytics.. been there forever)
  • play around with the page, there is some javascript that executes based on events on the page.

Pages

Note

for stats make sure to click through all side bar links to ensure all permutations of the page are working, as well check various date ranges including the custom range.

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 review from a team and eviljeff and removed request for a team March 17, 2025 12:16
@KevinMind KevinMind force-pushed the kevinmind/addons/15459 branch 6 times, most recently from 2532e22 to d7e1ab6 Compare March 18, 2025 15:50
@KevinMind KevinMind marked this pull request as draft March 18, 2025 15:51
@KevinMind KevinMind force-pushed the kevinmind/addons/15459 branch 4 times, most recently from 6ab657a to 6c323a3 Compare March 18, 2025 19:53
@KevinMind KevinMind marked this pull request as ready for review March 18, 2025 20:21
@KevinMind
Copy link
Contributor Author

@eviljeff CI is finally green. Ready to review/verify

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some random comments from a code review (I didn't read all the javascript changes - I presumed the code was functionally the same as before).
Next I'll test locally.

@eviljeff eviljeff self-requested a review March 19, 2025 17:44
@KevinMind KevinMind force-pushed the kevinmind/addons/15459 branch 3 times, most recently from 4131d6d to e56b4e5 Compare March 20, 2025 10:16
@eviljeff
Copy link
Member

on http://olympia.test/en-GB/developers/

on http://olympia.test/en-GB/developers/addons/

  • got a js error (as a warning, then an error):

Uncaught TypeError: $(...).exists is not a function
devhub.js:17
mightThrow chunk-VEIX3L4A.js:2009
process chunk-VEIX3L4A.js:2047
setTimeout handler*node_modules/jquery/dist/jquery.js/</Deferred/then/resolve/< chunk-VEIX3L4A.js:2072
fire chunk-VEIX3L4A.js:1805
add chunk-VEIX3L4A.js:1842
then chunk-VEIX3L4A.js:2085
Deferred chunk-VEIX3L4A.js:2136
then chunk-VEIX3L4A.js:2076
ready chunk-VEIX3L4A.js:2185
devhub.js:12
devhub.js:17:20

  • and a warning about source maps we might not care about:

Source map error: Error: request failed with status 404
Stack in the worker:networkRequest@resource://devtools/client/shared/source-map-loader/utils/network-request.js:43:9

Resource URL: http://olympia.test/en-GB/developers/%3Canonymous%20code%3E
Source Map URL: installHook.js.map

on reviewer tools: http://olympia.test/reviewers

  • I didn't see any problems on the queue or review pages

on stats: http://olympia.test/en-US/firefox/addon/spicy-pancake/statistics/

  • the xhrs to get the json to display on the charts 500s, and I can see tracebacks in the web container - but this is probably because I don't have auth to query bigquery, right? is there extra setup to get stats working?

I also tried to use the theme wizard, which uploads at the end of it, but it seems to cause some error in frontend (because all local traffic for server is routed through frontend) that ends up permanently 502'ing all requests until I make down && make up. I'd bet that that bug isn't introduced by this patch though.

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to follow-on my comments from testing.

  • the devhub landing 404s should be fixed (probably trivial)
  • the devhub /addons script error should be fixed, though I couldn't establish if it actually broke anything
  • I couldn't verify stats worked
  • uploads didn't work from the theme wizard, but likely not a regression from this patch.

@KevinMind KevinMind force-pushed the kevinmind/addons/15459 branch from 8c581e0 to c8ca8ca Compare March 20, 2025 12:12
@KevinMind
Copy link
Contributor Author

There is a pre-existing error where you end up serving stale files via nginx if the site-static directory is not empty. I believe this or some other caching issue is the reason for the js error

I have verified that stats is working at least the JS part.

I fixed the img paths.

I was able to get the static theme flow working. I think it might be related to the same caching issue which we should have a separate issue for.

@KevinMind KevinMind merged commit c90b679 into master Mar 20, 2025
42 checks passed
@KevinMind KevinMind deleted the kevinmind/addons/15459 branch March 20, 2025 14:03
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.

long gap of disabled page and nothing happening while waiting for Paypal popup on AMO
2 participants