-
Notifications
You must be signed in to change notification settings - Fork 3
feat: support async onShouldStartLoad requests #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| int lockIdentifier = [[RNCWebViewDecisionManager getInstance] setDecisionHandler:^(BOOL shouldStart) { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| if (!shouldStart) { | ||
| decisionHandler(WKNavigationActionPolicyCancel); | ||
| return; | ||
| } | ||
| allowNavigation(); | ||
| }); | ||
| }]; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The allowing code is moved into the allowNavigation helper to be reused below to keep the old fallback behavior
apple/RNCWebViewManager.m
Outdated
| RCTLogWarn(@"startLoadWithResult invoked with invalid lockIdentifier: " | ||
| "got %lld, expected %lld", (long long)lockIdentifier, (long long)_shouldStartLoadLock.condition); | ||
| } | ||
| [[RNCWebViewDecisionManager getInstance] setResult:result forLockIdentifier:(int)lockIdentifier]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kbulgakov-exo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
- WebView loads content normally when JS responds with
true - WebView rejects loading when JS responds with
false
guten-exodus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK nice
|
@guten-exodus Could you check whether this also addresses the pain points you had in Pay? |
The existing approach with locking the main thread to make a synchronous call asking JS "can I proceed with this URL" isn't reliable. Even the recent timeout bump to 500ms #47 didn't help, we still experience the loading issues. Bumping the timeout even more (for example with #48) would inevitably lead to performance degradation because of the locked thread. We should come up with a more reliable solution
The bottleneck
I've put some logs to diagnose what exactly is the bottleneck. The JS callback works super fast, but the problem is that sometimes it seems to be called after Native already makes the decision after the 500ms default timeout:
The solution
Let's mirror the async architecture of the
onShouldStartLoadcalls from https://github.com/react-native-webview/react-native-webview. We can easily backport most of the code.I recommend reviewing commit by commit
Notes:
WebViewDecisionManagerfrom upstream with minor formatting changesTesting
truefalse