Skip to content

LG-3756: Upgrade intl-tel-input from 16.0.7 to 17.0#4536

Merged
aduth merged 2 commits intomasterfrom
aduth-intl-tel-input-17
Dec 29, 2020
Merged

LG-3756: Upgrade intl-tel-input from 16.0.7 to 17.0#4536
aduth merged 2 commits intomasterfrom
aduth-intl-tel-input-17

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 28, 2020

Why: As a user, I want login.gov to not have unused or out of date apps, so that I can use the fastest and most secure app possible.

See changelog: https://github.com/jackocnr/intl-tel-input/blob/master/CHANGELOG.md

Specific upgrade conflicts:

  • Reliance on specific element IDs, now assigned uniquely by intl-tel-input
  • Reliance on duplicate list item element IDs, now assigned uniquely by intl-tel-input

**Why**: As a user, I want login.gov to not have unused or out of date apps, so that I can use the fastest and most secure app possible.

See changelog: https://github.com/jackocnr/intl-tel-input/blob/master/CHANGELOG.md

Specific upgrade conflicts:

- Reliance on specific element IDs, now assigned uniquely by intl-tel-input
- Reliance on duplicate list item element IDs, now assigned uniquely by intl-tel-input
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

/** @type {NodeListOf<HTMLLIElement>} */
const duplicates = iti.querySelectorAll(`.iti__standard[data-country-code="${countryCode}"]`);
duplicates.forEach((duplicateListItem) => {
duplicateListItem.parentNode?.removeChild(duplicateListItem);
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the cases when a list item has a null parent node? since we're querying from iti wouldn't every element have some parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what are the cases when a list item has a null parent node? since we're querying from iti wouldn't every element have some parent?

In most practical cases, never. Especially here since we're running querySelectorAll from a starting element, so at the very least that element would be the parent. I think TypeScript defaults to a generic Element|null type for the parent, since it can be a common case to "walk up the tree" using null as a condition to stop (e.g. Element#closest polyfill).

Another option here is to instruct TypeScript to treat it as though it exists:

Suggested change
duplicateListItem.parentNode?.removeChild(duplicateListItem);
/** @type {HTMLULElement} */ (duplicateListItem.parentNode).removeChild(duplicateListItem);

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah I didn't realize it was part of the method definition --- seems easier to just leave the null-safe navigator 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.

Oh yeah I didn't realize it was part of the method definition --- seems easier to just leave the null-safe navigator in there

Yeah, the main downside I see is that in practical terms, the transpiled output is quite verbose (example), though this feels pretty incidental and eventually temporary.

@aduth aduth merged commit 078b04d into master Dec 29, 2020
@aduth aduth deleted the aduth-intl-tel-input-17 branch December 29, 2020 14:12
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