Skip to content

Optimize initializer script#4233

Closed
aduth wants to merge 3 commits into
uswds:developfrom
aduth:aduth-uswds-init
Closed

Optimize initializer script#4233
aduth wants to merge 3 commits into
uswds:developfrom
aduth:aduth-uswds-init

Conversation

@aduth

@aduth aduth commented Jun 16, 2021

Copy link
Copy Markdown
Contributor

Description

Optimizes initializer script load and runtime performance, reducing size by 55%, from 869 bytes (520 bytes gzipped) to 362 bytes (231 bytes gzipped), and avoiding delays in removing the document loading class.

Additional information

The intended optimizations here are to:

  • Avoid Browserify for the initializer script. Browserify, as with most bundlers, has a static cost in the form of a runtime loader for resolving modules internally. You can see this on the first line of the unminified script. While some bundlers do better at this than others, since it's intended to be a small standalone script, it doesn't seem necessary to use a bundler at all.
  • Use events to verify USWDS script load instead of timers. Since we can assume that the global uswdsPresent will be assigned as a result of loading the USWDS script, we can monitor for load events on page assets (images, scripts, styles, etc) to respond to this immediately. This avoids any delays that could happen if the script finishes loading between the 100ms polling that exists today, and may be less intensive overall especially for slow page loads, where the code only runs for each asset load and not every 100ms regardless.

A few additional ideas were explored for more size savings, but not included here. Since revertClass is safely idempotent, and since most load events would happen only at the start of a page session, we could also consider to:

  • Avoid clearing timeout for fallback class revert after verified loaded: -22 bytes (-13 bytes gzipped)
    • revertClass would be a noop after the fallback delay if the class had already been removed previously
  • Avoid removing load event listener after verified loaded: -46 bytes (-16 bytes gzipped)
    • This would cause revertClass to be called for every load event after uswdsPresent is assigned, but similar to the above note, each of these would be a noop anyways.
  • Avoid the fallback altogether. At which point the full script could be... (130 bytes, 124 bytes gzipped)
    • document.addEventListener("load",function(){document.documentElement.classList.toggle("usa-js-loading",!window.uswdsPresent)},!0);

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

aduth added 2 commits June 16, 2021 11:05
**Why**: It's expected to be a standalone script. Bundler adds overhead in compiled output.
Comment thread src/js/uswds-init.js Outdated

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work, thanks for this!

Added some comments and will let the team chime in with their thoughts.

Comment thread config/gulp/javascript.js
const source = require("vinyl-source-stream");
const sourcemaps = require("gulp-sourcemaps");
const uglify = require("gulp-uglify");
const babel = require("gulp-babel");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some concerns about including gulp-babel since we're trying to get away from dependencies that don't update regularly.

Looking at their github, the last release was a year ago.

@aduth aduth Jun 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some concerns about including gulp-babel since we're trying to get away from dependencies that don't update regularly.

Looking at their github, the last release was a year ago.

gulp-babel doesn't really do a whole lot, since it delegates most of the work to the underlying @babel/core peer dependency. The way I see it, a main risk for it becoming outdated would be if it keeps up with a future 8.0.0 release (since it's pinned to ^7.0.0).

Since gulp works with plain Node streams, it could be substituted with a stream. Babel doesn't come with a stream-based API, but we could emulate one with a little wrapper:

diff --git a/config/gulp/javascript.js b/config/gulp/javascript.js
index b549fc027..8edb6ce99 100644
--- a/config/gulp/javascript.js
+++ b/config/gulp/javascript.js
@@ -1,2 +1,3 @@
 /* eslint-disable arrow-body-style */
+const { Transform } = require("stream");
 const buffer = require("vinyl-buffer");
@@ -11,3 +12,3 @@ const sourcemaps = require("gulp-sourcemaps");
 const uglify = require("gulp-uglify");
-const babel = require("gulp-babel");
+const babel = require("@babel/core");
 const merge = require("merge-stream");
@@ -32,3 +33,17 @@ gulp.task(task, () => {
     stream
-      .pipe(babel({ presets: ["@babel/preset-env"] }))
+      .pipe(
+        new Transform({
+          objectMode: true,
+          async transform(file, _encoding, callback) {
+            const { code } = await babel.transformAsync(
+              file.contents.toString(),
+              { presets: ["@babel/preset-env"] }
+            );
+            // eslint-disable-next-line no-param-reassign
+            file.contents = Buffer.from(code);
+            this.push(file);
+            callback();
+          },
+        })
+      )
       .pipe(rename({ basename }))

This is basically a reimplementation of what gulp-babel does anyways.

Another option: We could write the uswds-init.js in ES5-safe JavaScript, then skip Babel for that file altogether, then revert back to babelify for the remaining entrypoint.

Comment thread src/js/uswds-init.js Outdated
Comment thread src/js/uswds-init.js
revertClass();
document.removeEventListener("load", verifyLoaded, true);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this addition!

@mejiaj mejiaj requested a review from scottqueen-bixal June 17, 2021 20:21
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