USWDS - Core: Replace receptor behavior with custom optimized implementation #5793
Conversation
|
The build failure seems to be unrelated to the changes here, starting in #5233 and affecting all commits on Edit: See fix at #5795 |
- Reduces size of behavior by 66%, minimal component import (accordion) by 50% - Narrows scope to features in use - Avoid baked-in polyfills not necessary for current browser supports - Remove third-party dependencies to reduce maintenance overhead and security vulnerability attack surface area - Add TypeScript annotations for IDEs supporting code autocompletion (e.g. VS Code) ``` echo "import './packages/usa-banner/src/index.js'" | npx esbuild --bundle --minify | brotli-size ``` Before: 1.84 kB After: 896 B Diff: -944 B (-51.3%) ``` echo "import './packages/uswds-core/src/js/utils/behavior'" | npx esbuild --bundle --minify | brotli-size ``` Before: 1.43 kB After: 473 B Diff: -957 B (-66.9%)
|
This potentially resolves #6105 |
There was a problem hiding this comment.
Apologies that we’ve taken so long to get this scheduled.
We do appreciate this contribution — managing big PR’s is a pain point we haven't been able to address! We’ve taken this long to consider it because it's a complex pull request that includes a lot of changes to core functionality. We approach these types of PRs with caution so we don’t disrupt downstream teams. Your additions to the unit tests have helped a lot. So, in short, thanks for submitting this.
To help us speed up reviews like this in the future, it would help if you could use:
- Smaller PR's. Breaking out the work helps us reliably and quickly test one thing at a time, which we may need to do over multiple sprints, in order to work it into the rest of our in-flight work.
- JSDoc comments. This would be more in line with our code conventions, and also improves readability.
| /** | ||
| * @typedef {(target?: HTMLElement) => any} BehaviorLifecycle | ||
| */ | ||
|
|
||
| /** | ||
| * @name sequence | ||
| * @param {...Function} seq an array of functions | ||
| * @return { closure } callHooks | ||
| * @typedef {Record<string, any> & { init?: BehaviorLifecycle, teardown?: BehaviorLifecycle }} BehaviorProps | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {BehaviorProps & { | ||
| * on: BehaviorLifecycle, | ||
| * off: BehaviorLifecycle, | ||
| * add: BehaviorLifecycle, | ||
| * remove: BehaviorLifecycle, | ||
| * }} Behavior | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {( | ||
| * this: HTMLElement, | ||
| * event: K extends keyof HTMLElementEventMap ? HTMLElementEventMap[K] : Event | ||
| * ) => any} EventHandler | ||
| * @template {string} K | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {EventHandler<K> | Record<string, EventHandler<K>>} EventHandlerOrSelectorMap | ||
| * @template {string} K | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {Record<K, EventHandlerOrSelectorMap<K>>} Events | ||
| * @template {string} K | ||
| */ | ||
| // We use a named function here because we want it to inherit its lexical scope | ||
| // from the behavior props object, not from the module | ||
| const sequence = (...seq) => | ||
| function callHooks(target = document.body) { | ||
| seq.forEach((method) => { | ||
| if (typeof this[method] === "function") { | ||
| this[method].call(this, target); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * @name behavior | ||
| * @param {object} events | ||
| * @param {object?} props | ||
| * @return {receptor.behavior} | ||
| * @param {Events<K>} events | ||
| * @param {Partial<BehaviorProps>} props | ||
| * @template {string} K | ||
| * | ||
| * @return {Behavior} | ||
| */ |
There was a problem hiding this comment.
suggestion: Can we use JSDoc comments for this code?
References available in toggle-field-mask.js and debounce.js
| /** | ||
| * @typedef {(event: KeyboardEvent) => any} KeyboardEventHandler | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {Record<string, KeyboardEventHandler>} KeymapConfig | ||
| */ | ||
|
|
||
| /** | ||
| * @param {KeymapConfig} map | ||
| * @return {KeyboardEventHandler} | ||
| */ |
There was a problem hiding this comment.
suggestion: JSDoc comments here too, please.
| const listeners = Object.entries(events).flatMap(([eventTypes, handlers]) => | ||
| eventTypes.split(" ").map( | ||
| (eventType) => | ||
| /** @type {[keyof HTMLElementEventMap, EventHandler<any>]} */ ([ | ||
| eventType, | ||
| typeof handlers === "function" | ||
| ? handlers | ||
| : (event) => | ||
| Object.entries(handlers).some(([selector, handler]) => { | ||
| const target = event.target && event.target.closest(selector); | ||
| return target && handler.call(target, event) === false; | ||
| }), | ||
| ]), |
There was a problem hiding this comment.
polish: This code is a little hard to follow. We could improve it by making it more declarative and/or a using JSDoc comments.
There's probably a few candidates here we could split out to make the overall changeset a little smaller, with minimal overlap and maybe a bit of follow-on cleanup to remove the dependency once everything else is replaced:
I could go ahead and split out these items, if that'd make things easier. |
|
Closing in favor of smaller, targeted pull requests. I've also incorporated the review feedback into those pull requests. It also occurred to me that one of the reasons for making this as a single monolithic change is that it's hard to see the true value (performance impact) of this effort until the
Once all of those pull requests are merged, there'll still need to be a follow-on pull request to actually remove the dependency. |
|
Created #6291 to remove the dependency once everything is merged in. |
Summary
Reduced the JavaScript bundle size. By optimizing the implementation of core component code, overall JavaScript sizes are reduced.
Breaking change
This could be considered a breaking change in the unlikely circumstance that someone is using USWDS' internal
behaviormodule and expecting to leverage behaviors of the underlyingreceptorlibrary that are not implemented here, namely:types:delegate(selector)"Overall, I'd expect it to be quite unlikely anyone would be using these features, since it would require familiarity with
receptorsuch that I'd expect anyone interested in using them would usereceptordirectly rather than through USWDS'sbehaviorabstraction.Backwards-compatibility has been artificially preserved with the return value of
behaviorincluding additional function aliasesadd(alias foron) andremove(alias foroff).Problem statement
As a developer interested in optimizing their installation of USWDS, I expect that USWDS will keep overhead of common code to an absolute minimum, so that I can maximize optimization in cases where I'm interested in using one or a few components.
As a user interacting with a site using USWDS, I expect that pages will load quickly, so that I can access information without delay.
As a USWDS maintainer, I expect that the project minimizes its use of third-party dependencies in cases where such code is already heavily customized, so that I can avoid the maintenance burden associated with keeping those dependencies (including transitive dependencies) up-to-date and free from security vulnerabilities.
Solution
The changes proposed here reimplement
behaviorandkeymap, preserving existing functionality while removing underlying use of the third-partyreceptorlibrary.Why?
Object.assign,Element#closest)receptorwas 6 years ago)Performance Impact
The following examples use ESBuild and brotli-size-cli to simulate a real-world scenario of downloading a minified, bundled, and compressed JavaScript.
Scenario 1: Importing only
behaviorBefore: 1.43 kB
After: 473 B
Diff: -957 B (-66.9%)
Scenario 2: Importing only
keymapBefore: 1.18 kB
After: 413 B
Diff: -767 B (-65%)
Scenario 3: Importing a single component ("Best Case")
Before: 1.84 kB
After: 896 B
Diff: -944 B (-51.3%)
Scenario 4: Importing the full JavaScript package ("Worst Case")
Before: 22.2 kB
After: 20.9 kB
Diff: -1.3 kB (-5.9%)
Note: I believe the reason for lack of any difference here is that this pull request doesn't completely removereceptoryet, and there are imports referencing the package that may end up pulling in the whole library still (example). A follow-up pull request should seek to replacereceptor'skeymapfunctionality and remove the dependency altogether, which should prove to have an impact on the bundle size for the full package.Edit: I addressed this in 9924be3 by updating the import semantics, which prevents the full library from being pulled in, a temporary fix until the library can be removed altogether.
Edit 2: In additional commits starting from d55ee9c, I included the full replacement/removal of the
receptorlibrary, which further reduces the overall bundle size.Testing and review
Verify all unit tests pass:
If full unit test coverage cannot be guaranteed, it may be wise to review experience for individual components within the Storybook preview at http://localhost:6006