Adds Response Time out Error to RN SDKs #721
Conversation
packages/@magic-sdk/react-native-bare/src/react-native-webview-controller.tsx
Outdated
Show resolved
Hide resolved
| const timeout = setTimeout(() => { | ||
| this.messageTimeouts.delete(messageId); | ||
| throw createResponseTimeoutError(`Response timed out for method: ${methodType} with message id: ${messageId}`); | ||
| }, 5000); // 5-second timeout |
There was a problem hiding this comment.
Are we sure 5s is enough? We've had reports of calls to certain methods taking longer than 5s in regions like Asia and South Asia.
There was a problem hiding this comment.
Btw, do we know why the calls never return in some cases? I agree with having a time limit but if there's an issue on phantom that causes responses to hang we should probably look into that.
There was a problem hiding this comment.
@romin-halltari in this case, there could be multiple reasons for the delay. For the scope of this PR, the developer wants some indication that a response has been delayed, so we're focusing on just indicating that a delay has occurred as opposed to resolving any particular delay.
As for the 5 seconds, that was a random choice on my part. Given the realities of our users in Asia and South Asia I'll up it to 10 seconds. Thanks for the pointing that out!
| @@ -139,6 +140,18 @@ export class ReactNativeWebViewController extends ViewController { | |||
| }, [show]); | |||
|
|
|||
| const handleWebViewMessage = useCallback((event: any) => { | |||
| const data = JSON.parse(event.nativeEvent.data); | |||
There was a problem hiding this comment.
Is event.nativeEvent.data always defined? Because otherwise it would result in a crash, and that event: any typing above is not very reassuring 🤔
There was a problem hiding this comment.
It is not, hence the if-conditional check below 🤔
| // Setup timeout for message response | ||
| const timeout = setTimeout(() => { | ||
| this.messageTimeouts.delete(messageId); | ||
| throw createResponseTimeoutError(`Response timed out for method: ${methodType} with message id: ${messageId}`); |
There was a problem hiding this comment.
nit: I'd prefer we pass methodType and messageId as arguments to createResponseTimeoutError and create the error message inside that function, as it sounds like it would be the same everywhere.
Ariflo
force-pushed
the
ariflo-PDEEXP-233-add-response-timeout-to-RN-SDKs
branch
from
adb52d2 to
61f1514
Compare
Ariflo
force-pushed
the
ariflo-PDEEXP-233-add-response-timeout-to-RN-SDKs
branch
from
61f1514 to
8bafb75
Compare
📦 Pull Request
Adds a Response Timeout Error to our RN SDKs. See PDEEXP-233 for details.
✅ Fixed Issues
n/a
🚨 Test instructions
[Describe any additional context required to test the PR/feature/bug fix.]
Please 🚨 ONLY ADD ONE 🚨 of the following labels, failing to do so may lead to adverse versioning of your changes when published:
patch: Bug Fix?minor: New Feature?major: Breaking Change?skip-release: It's unnecessary to publish this change.Special Note
Please avoid adding any of the
Prioritylabels as they conflict with the labels above ☝️📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via: