[Bug] Link - Flagship crash mitigation efforts #573
Conversation
| const onOKPress = () => { | ||
| if (analytics?.onConfirm) analytics.onConfirm() | ||
| return Linking.openURL(url) | ||
| const onOKPress = async () => { |
There was a problem hiding this comment.
With these having been made async anyway to ensure the promise rejections are handled, not sure if we should also just do if (canOpenURL) first.
As noted, in storybook I observed no difference in behavior between canOpenURL and openURL in terms of getting warnings. Going into RN code, it mostly looks to just passthrough to native logic and canOpenURL and openURL at the JS level are identical (insofar as they seem to just pass to identically named functions via a native module).
That said, the doc for openURL does recommend canOpenURL for non-http URL's which does include our use (least for the lower try/catch, not this one).
There was a problem hiding this comment.
If the RN code and warnings look identical, I think the try/catch should be sufficient.
| console.log(JSON.stringify(e)) | ||
| console.log('Error: ' + e) |
There was a problem hiding this comment.
Included both because the error object is weird:
- Just straight logging
ewould not log anything to the console (not even that a console log event occurred, literally like noconsole.logwas present) - Concatenating with a string makes it work to log a string-wise error (when totally empty aligning with the firebase name error:
Invariant Violation: Invalid URL: cannot be empty) - The JSON stringify displays the object which seems to vary
- Sometimes it's a robust object (e.g.
httporhttpsURL) withmessageand other properties - Other times it's sparse (e.g. "") with just a
name(with less detail than the string concatenation above) andframesToPopwhatever that means
- Sometimes it's a robust object (e.g.
I'm not sure we actually care what this error is, but it seemed to make sense to generally allow consumer efforts to probe it.
There was a problem hiding this comment.
I like this addition. Should we check typeof e to determine whether to log the stringified version or not? i.e.
console.log(typeof e === 'Object' ? JSON.stringify(e) : `Error: ${e}\`)There was a problem hiding this comment.
The error output seems... weird.
Like, simultaneous output for a totally empty URL is:
LOG {"name":"Invariant Violation","framesToPop":1}
LOG Error: Invariant Violation: Invalid URL: cannot be empty
LOG Error opening URL:
where the first is the JSON.stringify(e) and second is "Error: " + e so the string is seemingly pulling data not present in the object. Maybe it's because it's a JS object and not JSON? But also as noted in the comment above that console.log(e) literally breaks anything from logging--it doesn't even have an empty LOG, it just completely prevents the console.log from working.
I didn't dig super deeply into it except finding this StackOverflow on typing it where it also seems pretty arbitrary, although their suggestions are splitting it--but as noted in at least one error it's seemingly both because the string contains data the object doesn't which makes no sense.
As this is in our story and a consumer can send what they like + the observed behavior + the time sensitivity, didn't seem worth digging deeper into how forcing it to be a string somehow seems to pull out more info than exists in the object form for a case as clearly that's genuinely odd behavior.
There was a problem hiding this comment.
As a circle-back on this with more testing, a few notes:
- It can be OS dependent--for non-empty URL's, iOS/Android yield different error objects which have differing OS-level code traces
- From these OS-level errors, the string form looks to always output what is in the
messageskey of the error object - For empty URLs, it hits the RN
Invariant Violation: Invalid URL: cannot be emptyerror and an error object that has no (shown)messageskey
| | { | ||
| /** Name of preset icon to use {@link IconMap} **/ | ||
| name: keyof typeof IconMap | ||
| noIcon?: never |
There was a problem hiding this comment.
The addition of the noIcon prop works, but what are your thoughts on allowing a 'none' value to be passed to the name prop to achieve the same thing without the addition of a new prop?
There was a problem hiding this comment.
I originally tried it that way with having name be [existing typing] | 'no icon', but it was causing a huge headache because although the name list seems like strings when calling, it's actually the SVG-mapping components (React.FC<SvgProps>) which is an object.
Most notably this was causing issues around trying to pass the IconProps sent by the consumer because the spread, {..._icon}, to pass the property overrides (if present) didn't want to allow conditionalizing when the name prop was an object or string. Trying to manually set the name and then ..._icon the rest then also ran into issues with the exclusionary typing for name and svg props.
Ultimately, the headache was proving sufficiently annoying to try resolving that I changed it up. One key factor was: I didn't want to pull it out of what is passed to the Icon to do more meaningful manipulation of the props as that seemed like it would weaken the defensiveness by have more non-render-time conditionality in the Link that this defensive change was specifically trying to avoid.
There was a problem hiding this comment.
Okay let's keep the prop then. Doesn't seem worth the headache. It'll probably be used sparingly, maybe internally only.
There was a problem hiding this comment.
Thinking more on this, it would potentially work fine if actually adding 'none' directly to IconMap--in retrospect, perhaps the object it was referring to wasn't the React.FC<SvgProps> but the object of all the icons typed that way. But that would involve a weird edit to the buildIcons script and not sure what it'd be typed as or if that'd cause other problems with it not being React.FC<SvgProps> like all the other icons in IconMap.
| console.log(JSON.stringify(e)) | ||
| console.log('Error: ' + e) |
There was a problem hiding this comment.
I like this addition. Should we check typeof e to determine whether to log the stringified version or not? i.e.
console.log(typeof e === 'Object' ? JSON.stringify(e) : `Error: ${e}\`)| @@ -96,6 +93,7 @@ export type LinkAnalytics = { | |||
| onPress?: () => void | |||
| onConfirm?: () => void | |||
| onCancel?: () => void | |||
| onOpenURLError?: (e?: unknown, url?: LinkProps['url']) => void | |||
There was a problem hiding this comment.
Is e always string | object, or can it sometimes be neither?
There was a problem hiding this comment.
As noted in other comment, I didn't dig exhaustively deeply, but readily found a StackOverflow discussion that seemed to suggest errors can be whatever they (whomever coded the error) want and unknown seems to be the recommended typing.
| const onOKPress = () => { | ||
| if (analytics?.onConfirm) analytics.onConfirm() | ||
| return Linking.openURL(url) | ||
| const onOKPress = async () => { |
There was a problem hiding this comment.
If the RN code and warnings look identical, I think the try/catch should be sufficient.
| onCancel: () => console.log('Analytics event: Canceled'), | ||
| onPress: () => console.log('Analytics event: Pressed'), | ||
| onConfirm: () => console.log('Analytics event: Confirmed'), |
There was a problem hiding this comment.
What do you think about adding some promises to pass to one of stories to test our async logic in useExternalLink? Could just be a fake promise function that logs to console after a setTimeout or something.
function somePromise() {
return new Promise((resolve) => {
setTimeout(() => {
console.log("This message is logged after a 2-second delay");
resolve();
}, 2000);
});
}There was a problem hiding this comment.
Not sure I follow. Like--adding async analytics events? Seems odd for an analytics event to be async.
The conversion to async was around the fact that Linking.openURL is inherently async*, but for some reason seems to work just fine for the error free path without actually being async. However, to handle the promise rejection correctly, it does require async behavior to wait for the OS to fail to find a suitable way to open the URL. Aside from weird async analytics functions, I don't believe there's anything a Link component consumer could do to otherwise to probe the async adjustments in useExternalLink since it only takes the URL string and Alert prompt override strings--more custom behavior (e.g. async function not analytics) from the app would be custom onPress logic and circumvent the hook entirely.
*From the RN doc: The method returns a Promise object. If the user confirms the open dialog or the url automatically opens, the promise is resolved. If the user cancels the open dialog or there are no registered applications for the url, the promise is rejected.
Description of Change
Efforts to make code more defensive against crashes:
url, but validated no longer getting unhandled promise warnings for various edge cases (see testing section below)tel:) such that theurlis not an empty string even if the consumer passed an empty string which results in weird behavior, but no warning of a failureUpdated 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:
tel:prefix yields expected hit of new analytic eventsms:prefix yields expected hit of new analytic eventhttps://www.yielded a server cannot be found errorhttps://wwwyielded the samehttps://yielded a cannot open page errorhttps:yielded the samehttpsyielded a "dead" click but hit new analytic event w/ error object and error message:Error: Unable to open URL: file:///private/var/containers/Bundle/Application/0CAB9745-B322-4A0B-90C7-63F66F13ACA9/Expo%20Go.app/httpshttpyielded the samehttyielded the same (no confirm dialog anymore, conditional onhttpstart)Invariant Violation: Invalid URL: cannot be emptyhttps://www.yielded a can't find site errorhttps://wwwyielded the samehttps://yielded the browser opening with URLabout:blankhttps:yielded the samehttpsyielded a "dead" click but hit new analytic event w/ robust error object and error message:Error: Could not open URL 'https': No Activity found to handle Intent { act=android.intent.action.VIEW dat=https flg=0x10000000 }httpyielded the samehttyielded the same (no confirm dialog anymore, conditional onhttpstart)Invariant Violation: Invalid URL: cannot be emptyAn 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:
changeloglabel applied if it's to be included in the changelogPublish
If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:
maininto branchmain