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

OBS-247: Use ESBuild for JS and remove django-pipeline #6915

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

toufali
Copy link
Contributor

@toufali toufali commented Feb 23, 2025

Refactor the front-end JavaScript codebase to adopt ECMAScript Modules (ESM) as the standard for module management.

Use ESBuild to bundle JS with minification, tree-shaking, etc, removing dependency on django-pipeline. Django-pipeline is troublesome, blocking updates to other dependencies, and the project isn’t well maintained. Using ESBuild to bundle JS helps modernize the Socorro front-end.

To test this PR:
Run the app in both development and production modes following this test plan: https://mozilla-hub.atlassian.net/wiki/spaces/CS1/pages/605028396/Socorro+manual+test+plan#Webapp

  1. just build and look out for an ESBuild summary that ends similar to "⚡ Done in 218ms". The collectstatic summary should follow, similar to "328 static files copied to '/app/webapp/static', 4 unmodified, 23 post-processed."
  2. just setup
  3. just run
  4. You should now be running in development. Follow test plan to ensure app functions as expected.
  5. To test production, ctrl-c to stop the server.
  6. docker compose run --service-ports webapp shell
  7. npm run build --prefix webapp (this step might not be needed if files haven't changed since the last build.)
  8. DEBUG=False bash bin/run_service_webapp.sh
  9. You should now be running in production. Follow test plan to ensure app functions as expected.

TODO:

  • resolve failing test -- I'm not understanding why this is failing:
class Test404:
    def test_handler404(self, client):
        response = client.get("/fillbert/mcpicklepants")
        assert response.status_code == 404
        assert "The requested page could not be found." in smart_str(response.content)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is a busy file including imports of files that import old 3rd-party deps (D3, metrics-graphics). I was unable to test this view.

@toufali toufali requested a review from willkg February 24, 2025 00:24
@willkg
Copy link
Contributor

willkg commented Feb 26, 2025

We have pytest set up to report Python warnings as errors. We're now seeing a test failure because this PR changes how static files get served in the local dev environment and that changed some of the specifics of the ResourceWarning.

We ignore this warning by specifying a pattern in the webapp/pytest.ini file. I haven't tested this, but I think we can update the io class in this line from _io.FileIO to _io.BufferedReader which will match the adjusted warning and that'll make the failure go away:

# some django tests kick up unclosed file warning for favicon
# https://bugzilla.mozilla.org/show_bug.cgi?id=1909011
ignore:unclosed file <_io.FileIO name='/app/webapp/crashstats/crashstats/static/img/favicon.ico':ResourceWarning

Docs on pytest and filterwarnings:

https://docs.pytest.org/en/stable/how-to/capture-warnings.html

The pattern format for filterwarnings value is my least favorite thing in the universe. For some reason, it takes me like 50 tries to get a pattern line that matches correctly.

@toufali toufali force-pushed the OBS-247/use-esbuild-for-js branch from c2b0a39 to 7f7c8d4 Compare February 27, 2025 04:14
@toufali
Copy link
Contributor Author

toufali commented Feb 27, 2025

Thanks @willkg for the tip. I ended up needing a second ignore line, duplicating the first with BufferedReader as you suggested.

@toufali toufali marked this pull request as ready for review February 27, 2025 04:25
@toufali toufali requested a review from a team as a code owner February 27, 2025 04:25
Copy link
Contributor

@willkg willkg left a comment

Choose a reason for hiding this comment

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

Tests pass.

Then I started going through the webapp manual test plan.

I processed some crash reports so I had crash data to work with.

Front page looks fine. I clicked around through the navigation menus and didn't see any HTTP 404s or JS errors.

Going to the Top Crashers report, I get this error in the dev tools console:

jQuery.Deferred exception: $(...).tablesorter is not a function @http://localhost:8000/static/topcrashers/js/topcrashers.min.js:3:60420
j@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:31259
is</</Deferred/then/_/</be<@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:31579
setTimeout handler*is</</Deferred/then/_/<@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:31794
g@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:29326
fireWith@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:30084
fire@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:30120
g@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:29326
fireWith@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:30084
ready@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:33205
setTimeout handler*is</<@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:33434
is</<@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:925
is<@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:1042
ts/<@http://localhost:8000/static/crashstats/js/crashstats.min.js:1:462
@http://localhost:8000/static/crashstats/js/crashstats.min.js:6:56993
 undefined [jquery.js:4046:17](http://localhost:8000/node_modules/jquery/dist/jquery.js)

Uncaught TypeError: $(...).tablesorter is not a function
    <anonymous> topcrashers.js:13
    jQuery 12
    ts crashstats.min.js:1
    <anonymous> jQuery

If I go to the signature report, I get this error in the dev tools console:

Uncaught ReferenceError: SignatureReport is not defined
    <anonymous> signature_tab_reports.js:11
[signature_tab_reports.js:11](http://localhost:8000/crashstats/signature/static/signature/js/signature_tab_reports.js)
    <anonymous> signature_tab_reports.js:11

If I go to Super Search and do a search, I see this error in the dev tools console:

Uncaught TypeError: $(...).tablesorter is not a function
    y search.js:101
    jQuery 6
    _ search.js:158
    c search.js:57
    ge search.js:453
    jQuery 9
    ge search.js:451
    le search.js:378
    he search.js:416
    G dynamic_form.js:120
    jQuery 8
    G dynamic_form.js:112
    ne search.js:425
    Ue search.js:555
    <anonymous> search.js:558
    jQuery 12
    ts crashstats.min.js:1
    <anonymous> jQuery
search.js:101:28
    y search.js:101
    jQuery 6
    _ search.js:158
    c search.js:57
    ge search.js:453
    jQuery 9
    ge search.js:451
    le search.js:378
    he search.js:416
    G dynamic_form.js:120
    jQuery 8
    G dynamic_form.js:112
    ne search.js:425
    Ue search.js:555
    <anonymous> search.js:558
    jQuery 12
    ts crashstats.min.js:1
    <anonymous> jQuery

I think we need to process some crashes and then go through the webapp manual test to make sure everything is working.

@toufali toufali force-pushed the OBS-247/use-esbuild-for-js branch from a3e1c30 to ecf4f71 Compare March 7, 2025 03:16
@willkg
Copy link
Contributor

willkg commented Mar 10, 2025

@toufali : The commits reference Jira issues that don't have any details. Can you flesh those out by adding descriptions, accepted criteria, priorities, etc?

@toufali
Copy link
Contributor Author

toufali commented Mar 11, 2025

@toufali : The commits reference Jira issues that don't have any details. Can you flesh those out by adding descriptions, accepted criteria, priorities, etc?

Thanks @willkg – Jira issues have been updated.

I've done a few more passes since your last review and have confirmed the JS works once the application is populated with crash data. Would you mind testing again when you have a chance? In particular, I wasn't sure how to validate the "testdrive" scripts, which I believe relate to the API tokens?

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.

2 participants