Skip to content

Extract Cleave.js-dependant JavaScript from application pack#5435

Merged
aduth merged 5 commits intomainfrom
aduth-extract-cleave
Sep 22, 2021
Merged

Extract Cleave.js-dependant JavaScript from application pack#5435
aduth merged 5 commits intomainfrom
aduth-extract-cleave

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Sep 22, 2021

Why: Reduce minified+gzipped bundle size of hot-path application.js by ~25%, and further establish convention of partial/component-specific JavaScript packs.

Cleave.js is not small: https://bundlephobia.com/package/cleave.js@1.6.0

Asset Before After
application.js 78.1 KiB 56.6 KiB
application.js.br 22 KiB 16.7 KiB
application.js.gz 24.9 KiB 18.8 KiB

(Generated by disabling chunk splitting and running NODE_ENV=production RAILS_ENV=production ./bin/webpack)

**Why**: Reduce minified bundle size of hot-path application.js by ~25%, and further establish convention of partial/component-specific JavaScript packs.
@@ -0,0 +1,23 @@
import Cleave from 'cleave.js';

const SELECTOR_CONFIGS = {
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 excludes .dob from form-field-format.js, which no longer exists as of #4852.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 5406719, I also removed this from form-validation.js, along with state_id_number which was similarly removed as of #3682.

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

👏🏼 👏🏼

love this

Comment on lines +20 to +23
Object.entries(SELECTOR_CONFIGS)
.map(([selector, config]) => [document.querySelector(selector), config])
.filter(([element]) => element)
.forEach(([element, config]) => new Cleave(element, config));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To confirm: The Cleave constructor can only handle one element, even though a class selector could match multiple elements. This is true regardless if we pass the selector (previous) or element (current) to the constructor.

See: https://github.com/nosir/cleave.js/blob/e3fa6f3/src/Cleave.js#L29-L36

If we ever need to support multiple instances of elements on the page, we could consider constructing each:

Suggested change
Object.entries(SELECTOR_CONFIGS)
.map(([selector, config]) => [document.querySelector(selector), config])
.filter(([element]) => element)
.forEach(([element, config]) => new Cleave(element, config));
Object.entries(SELECTOR_CONFIGS)
.map(([selector, config]) => [document.querySelectorAll(selector), config])
.filter(([elements]) => elements.length)
.forEach(([elements, config]) => elements.forEach((element) => new Cleave(element, config)));

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

</div>
<% end %>
<%= render 'idv/doc_auth/start_over_or_cancel', step: 'ssn' %>
<%= javascript_packs_tag_once('ssn-field') %>
Copy link
Contributor

Choose a reason for hiding this comment

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

the ssn field markup is basically identical (same form field name, same translation) to the one in ssn_update, if we're going to have a separate JS include for this file would it make sense to extract a partial for that field and put the JS pack tags in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ssn field markup is basically identical (same form field name, same translation) to the one in ssn_update, if we're going to have a separate JS include for this file would it make sense to extract a partial for that field and put the JS pack tags in there?

Yeah, I think we might be able to consolidate to one partial between creating and updating SSN. Even then, it could still make sense to extract a partial just for the input field as well, so that the markup and JavaScript are packaged as a self-contained unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted the SSN field in b9ca762. I started consolidating the SSN views as well, but it's a bit more work than I'd like to take on here, so probably worth a follow-up.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth aduth merged commit e1383df into main Sep 22, 2021
@aduth aduth deleted the aduth-extract-cleave branch September 22, 2021 19:40
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.

3 participants