Skip to content

Remove unnecessary use of DOMContentLoaded#11127

Merged
aduth merged 2 commits intomainfrom
aduth-rm-unnecessary-dom-content-loaded
Aug 22, 2024
Merged

Remove unnecessary use of DOMContentLoaded#11127
aduth merged 2 commits intomainfrom
aduth-rm-unnecessary-dom-content-loaded

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Aug 21, 2024

🛠 Summary of changes

Updates code which binds events to DOMContentLoaded to execute immediately.

This event is typically used to ensure that all content is present on a page before executing JavaScript, in case the script is loaded earlier than the elements it references. Packs are always loaded as the last content of every page (source), so it's not necessary to wait before referencing these elements.

This also relates to #11096 and #11126, where defer has the effect of delaying DOMContentLoaded being fired, unlike asynchronous scripts. We can offset this by not relying on the DOMContentLoaded.

The DOMContentLoaded event fires when the HTML document has been completely parsed, and all deferred scripts (<script defer src="…"> and <script type="module">) have downloaded and executed. It doesn't wait for other things like images, subframes, and async scripts to finish loading.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Document/DOMContentLoaded_event

📜 Testing Plan

Verify no regressions in affected code:

  • Mock device profile on the SSN step of IdV Edit: Reverted in 509ccee
  • SSN formatting and visibility toggle on SSN step of IdV
  • State-specific guidance for IPP

changelog: Internal, Performance, Remove unnecessary use of DOMContentLoaded
@aduth aduth requested a review from mitchellhenke August 21, 2024 20:01
Copy link
Copy Markdown
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.

Is it worth adding a lint to prevent adding DOMContentLoaded listeners?

function formatSSNFieldAndLimitLength() {
const inputs = document.querySelectorAll<HTMLInputElement>('input.ssn-toggle[type="password"]');

if (inputs) {
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.

For awareness, I snuck an extra change in to remove this redundant condition. querySelectorAll will always return an array, and an array is always truthy.

See: https://dorey.github.io/JavaScript-Equality-Table/#if-statement

@aduth aduth merged commit 35adb57 into main Aug 22, 2024
@aduth aduth deleted the aduth-rm-unnecessary-dom-content-loaded branch August 22, 2024 17:58
@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented Aug 22, 2024

Is it worth adding a lint to prevent adding DOMContentLoaded listeners?

Possibly, yeah. Based on what we discovered here with legitimate usage, I might hold off on that until we remove all existing usage.

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