department-of-veterans-affairs / va-mobile-library

https://department-of-veterans-affairs.github.io/va-mobile-library/
ISC License
1 stars 0 forks source link

[Bug] Link - Flagship crash mitigation efforts #573

Closed TimRoe closed 3 weeks ago

TimRoe commented 3 weeks ago

Description of Change

Efforts to make code more defensive against crashes:

  1. Updated Link _onPress logic to no longer be async
    • Likely legacy carryover from flagship component that uses async for adding to calendar (as it needs to query device calendar permissions), design system Link omitted calendar as it necessitates native code so should not be relevant
  2. Updated Icon component to allow a null passthrough and made the Link use it
    • Was noted as a potential error spot for the more/fewer hooks between renders as the no icon Link would return null while an icon-having Link would return the Icon component which contains hooks
    • With the null passthrough, it will always be "rendered" so the hooks should remain consistent
    • This adjustment also necessitated pulling the aligning the icon with the first row of text logic into the Icon component--I believe there's already a ticket outstanding to centralize that logic anyway and now it is so Alert/Snackbar/Checkbox can be updated accordingly if no issues are noted
  3. The useExternalLink hook was updated to handle the unhandled promise errors from RN's Linking.openURL function when the URL is faulty
    • Entailed making the innards use async/await and try/catches
    • Should technically be impossible to fail for all Link types except url, but validated no longer getting unhandled promise warnings for various edge cases (see testing section below)
      • Other Link types apply a prefix (e.g. tel:) such that the url is not an empty string even if the consumer passed an empty string which results in weird behavior, but no warning of a failure

Updated 2 unit tests that broke from the changes in a manner expected based on the changes. Ticket #574 was created to more fully update unit tests separately due to time sensitivity of the changes.

Testing

Defensive code 1 above:

Defensive code 2 above:

Defensive code 3 above:

An alpha build was considered for testing, but ultimately deemed not needed as these changes are ultimately self-contained to logic that is capable of being hit within Storybook. Additionally, it is aiming for quick turn-around with the app and app-level QA should cover any testing that would've been performed with an alpha build.

PR Checklist

Code reviewer validation:

Publish

If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge: