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

MNTOR-1171: Code Splitting #2987

Merged
merged 4 commits into from Apr 14, 2023
Merged

MNTOR-1171: Code Splitting #2987

merged 4 commits into from Apr 14, 2023

Conversation

toufali
Copy link
Member

@toufali toufali commented Apr 13, 2023

References:

Jira: MNTOR-1171

Description

Currently, client-side javascript files are all bundled and loaded for every page regardless of need.

Example: on Landing page, we currently load a bundle that includes Landing page-specific script and site-wide scripts, as well as unnecessary scripts from Breaches page and Settings page.

This PR aims to separate global modules used for every page from page-specific modules that should be loaded as-needed.

TODO:

  • use dynamic import to auto-load page-specific modules
  • rename script and stylesheet files to camelCase, in order to integrate with auto-load
  • refactor/simplify dialog module to work with camelCase filenames
  • add a linter to enforce camelCase filenames
  • explore adding new "pages" folder to distinguish between full page html and smaller partial snippets
  • explore if any benefit to adding new "dialogs" folder to hold full page html that lives within dialog layout
  • update readme

How to test

Regression testing on every page

Checklist (Definition of Done)

  • Localization strings (if needed) have been added.
  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug.
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

@toufali toufali marked this pull request as ready for review April 14, 2023 06:56
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

The changes look good, and I'm looking forward to that darned error being prevented from now on!

I wonder, while we're in conflictopalooza, and if it's not too much work, if you could also adjust the controller file paths to align with their routes? So that have e.g. /src/controllers/api/v1/requestBreachScan.js instead of /src/controllers/requestBreachScan.js. I have some minor but consistent struggles to find the right controller, esp. distinguishing between backend and frontend routes. But if that gets you too deep in the weeds, then never mind, we can deal with it some other time.

@@ -2,8 +2,10 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

/* Import global styles that apply to every page.
* Partial-specific stylesheets are auto-loaded based on filename and should not be added here */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good callout!

src/views/guestLayout.js Show resolved Hide resolved
@groovecoder
Copy link
Member

I've noticed that Monitor sends cache-control: public, max-age=0 on CSS files so there's no browser caching at all. Claim: Code-splitting the CSS into more separate network requests without fixing CSS caching will actually make performance WORSE. It will save bandwidth, but increase the total latency needed to serve monitor pages. 😢

Copy link
Collaborator

@flozia flozia left a comment

Choose a reason for hiding this comment

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

Looking good!

@@ -42,6 +42,7 @@ const mainLayout = data => `
<link rel='apple-touch-icon' href='/images/apple-touch-icon.webp' sizes='180x180'>

<script src='/js/index.js' type='module'></script>
${data.skipPartialModule ? '' : `<script src='/js/partials/${data.partial.name}.js' type='module'></script>`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (non-blocking): If we do include a specific script for a partial, is it pretty safe to say that a page does not work without it? Should we add a note to pages (the dashboard for example) that are impacted by JS being disabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

@flozia do you mean accommodating no-script users? I think it could be helpful for promotion pages (Landing for ex), but if script is disabled for landing, the newly added form simply doesn't work – the entire rest of the page should still function as a promo page sharing content. Perhaps we could audit the site with script turned off to see what the impact is – at a certain point (past the landing) our site is just not usable without script...

Copy link
Collaborator

@mansaj mansaj left a comment

Choose a reason for hiding this comment

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

LGTM!

@toufali
Copy link
Member Author

toufali commented Apr 14, 2023

I've noticed that Monitor sends cache-control: public, max-age=0 on CSS files so there's no browser caching at all. Claim: Code-splitting the CSS into more separate network requests without fixing CSS caching will actually make performance WORSE. It will save bandwidth, but increase the total latency needed to serve monitor pages. 😢

Thanks @groovecoder for pointing this out – I've added this as a separate ticket with high-priority.

@toufali
Copy link
Member Author

toufali commented Apr 14, 2023

also adjust the controller file paths to align with their routes? So that have e.g. /src/controllers/api/v1/requestBreachScan.js instead of /src/controllers/requestBreachScan.js

@Vinnl I love this idea. I think @mansaj has been primarily adding API files, I wonder if there's anything additional to consider?

In effort to get this PR in before new devs start working on Monday, I'll merge this now and create a separate ticket for the API path matching and cc you.

@toufali toufali merged commit 7391fee into main Apr 14, 2023
9 checks passed
@toufali toufali deleted the MNTOR-1171/code-splitting branch April 14, 2023 21:25
@Vinnl Vinnl mentioned this pull request May 5, 2023
11 tasks
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.

None yet

5 participants