support message passing and keeping native WebView in memory when react component is unmounted #2469
Conversation
donaldchen
changed the title
|
In 03d597c , I added the changes from discord#4 (and changes from some of the commits on https://github.com/discord/react-native-webview/commits/11.18.1-discord-1 that came after that PR ). While integrating this with the Discord iOS app, I realized that I needed the ability to release the WebView with a web-view-key via a static method when the React screencap-2022-05-05T175658.393Z.mp4 |
|
Overall I really like this approach, at work we have been developing this feature set as well. The API is very similar and we have it fully working on Android. Hopefully we can work together to get this feature set stable and in the open source codebase here. |
|
@mattapperson , that's exciting! Do you have this working on iOS also, or do you only have it working on Android so far? Do you have the code for your changes publicly viewable (e.g. in a fork of the repo)? If not, are you willing to share that e.g. via a PR into this repo or into https://github.com/discord/react-native-webview/commits/11.18.1-discord-1 ? I'm curious about the similarities and differences between your current approach versus my iOS changes in this PR and my proposed Android changes in the PR description |
|
We currently have it working on both iOS and Android, but have a few bugs to work out in our iOS implementation. Nothing is public yet for this for our effort, but I hope to very soon. |
@mattapperson , sounds good! Feel free to tag me when you all are ready to make that work public. I'm interested in collaborating to get the feature set stable and merged into this open-source / upstream react-native-webview code |
|
I don't have time to look at this right at the moment, but this sounds really interesting. I wanted to give the one bit of early feedback that I do prefer your original plan of "wait until Android is also ready," since there's the potential that finishing Android could involve tweaking iOS/API for consistency/etc. But it sounds like you might be able to get some help with that. 😄 |
|
@TheAlmightyBob , thanks for the feedback! @Titozzz and @jamonholmgren , do you have any thoughts on the proposal in the PR description? |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
@TheAlmightyBob , @Titozzz , and @jamonholmgren , can we remove the "stale" label from this PR? @nealmanaktola and I have been making more changes to this in our fork of the repo here https://github.com/discord/react-native-webview/commits/11.18.1-discord-1 . I expect to eventually update this PR once our iOS (and eventually Android) implementations of this are more stable |
|
Fyi I'm in a major rewrite of the lib for the new architecture, so you'll probably have a lot of conflicts to handle. |
@Titozzz , in the new architecture, on Android, is there a plan to make the Android |
|
I have implemented this feature for Android by referring to this PR and discord#2 (These helped us a lot). I've forked (Note: This cou929#1 branch is just for sharing, so this is not the actual code I internally use in our production. Also, in this branch, we only implemented the requirements we needed, which might be not sufficient for common use cases.) I hope this PR is going to be merged into the master eventually due to the maintenance cost of our forked branch. But it looks like it hasn't been updated in months. Do you have any plans for this PR? Is there anything I can do to help move forward? |
apple/RNCWebView.m
Outdated
| sharedWKWebViewDictionary[_webViewKey] = _webView; | ||
| NSMutableDictionary *sharedRNCWebViewDictionary= [[RNCWebViewMapManager sharedManager] sharedRNCWebViewDictionary]; | ||
| sharedRNCWebViewDictionary[_webViewKey] = self; |
There was a problem hiding this comment.
What's the respective purpose of these 2 different dictionaries?
There was a problem hiding this comment.
At a high level, the RNCWebView's lifecycle is one-to-one with the lifecycle of the corresponding react-component, and the WKWebView's lifecycle is not one-to-one with the lifecycle of the corresponding react-component. sharedWKWebViewDictionary lets us keep the WKWebView alive when the first react-component unmounts and when its corresponding RNCWebView gets destroyed.
sharedRNCWebViewDictionary exists because it's possible to begin with one RNCWebView and one WKWebView for a given web view key. Then it's possible to mount a second react component for the webview with the same webview key before the first react component for the first RNCWebView unmounts. At this point, there will be two RNCWebViews with the same web view key but only one WKWebView. In didMoveToWindow for the second RNCWebView, we need to call
RNCWebView *rncWebView = sharedRNCWebViewDictionary[_webViewKey];
if (rncWebView != nil) {
[self removeWKWebViewFromSuperView:rncWebView];
}
and remove the WKWebView from the first RNCWebView before adding it as a child of the second RNCWebView. Otherwise, the app will crash.
There was a problem hiding this comment.
Oh that's interesting. Thanks for the explanation.
So, if you were you just put two WebViews on a screen with the same key, I'm guessing whichever one mounts last will "win" and the other will just be blank?
There was a problem hiding this comment.
So, if you were you just put two WebViews on a screen with the same key, I'm guessing whichever one mounts last will "win" and the other will just be blank?
Yeah, that's what I expect to happen. Ideally, the react / JS part of the app will typically be written in a way such that there is only one web view component on screen at a time for a given key. However, I've run into this scenario with two webview components with the same key in my earlier testing, and I felt that blanking was better than crashing
There was a problem hiding this comment.
I'm in the process of updating the PR description, and I came across this other reason, which I had documented in an earlier version of the PR description, for having two maps: one for WKWebViews and one for RNCWebViews.
We also have a map of web view keys to
RNCWebViews, so we can remove the correct observer from theWKWebView. If we don't remove the observer when we change theWKWebView's parent, the app will crash with anEXC_BAD_ACCESSerror.
This may have been one source of crashes before we added that RNCWebView map.
cc @nealmanaktola , since we previously discussed whether we needed the two maps on iOS
There was a problem hiding this comment.
I just noticed that updated comment you added, which I appreciate.
I was wondering though, could you not check the WKWebView's superview property for the first RNCWebView rather than looking it up in a dictionary?
There was a problem hiding this comment.
@TheAlmightyBob , you're right! We should be able to check the WKWebView's superview property for the first RNCWebView. In these two new commits
, I've updated our iOS and Android implementations to (a) remove the map of RNCWebViews and (b) get the RNCWebView through the internal WebView's parent before detaching the internal WebView from its parent
There was a problem hiding this comment.
Thanks so much for linking the commits! I see the map still exists on Android, it's just used in fewer places. Was keeping it intentional, or is that change just not finished yet?
src/WebViewTypes.ts
Outdated
| keepWebViewInstanceAfterUnmount?: boolean; | ||
|
|
||
| /** | ||
| * When keepWebViewInstanceAfterUnmount is true, if two React components use the same | ||
| * key for the WebView, they will use the same native WebView instance. | ||
| */ | ||
| webViewKey?: string; |
There was a problem hiding this comment.
What happens if keepWebViewInstanceAfterUnmount is true but webViewKey is undefined?
There was a problem hiding this comment.
As currently written, then in that scenario, the WKWebView will have the default behavior: its lifecycle will be roughly one to one with the lifecycle of the corresponding react component.
keepWebViewInstanceAfterUnmount needs to be true and webViewKey must be non-nullish for the WKWebView to stay alive after its corresponding react component unmounts. I wrote it this way to make the behavior more explicit. It's theoretically more flexible because then we can use the webViewKey for other cases too if necessary. I haven't actually thought of concrete examples that would need just the webViewKey though, so I'm open to changing this if that's simpler.
An alternative option is to remove keepWebViewInstanceAfterUnmount and then assume that if webViewKey is non-nullish, then we should keep the WKWebView alive after react-component-unmount.
There was a problem hiding this comment.
The alternative option is what I was originally thinking, but being explicit isn't a bad idea as long as it's documented that you need to use both.
The other use of webViewKey that I've been thinking of (and mentioned in #2640) would be to leverage this for supporting popup windows.... e.g., if web content calls window.open, the native "create window" handler could potentially create a new WKWebView in the dictionary and raise an event with the (generated?) key, which the app could then use (in RN code) to render a new WebView with that key. Theoretically, this could mean that the two WebViews would be connected as they would be in a browser.
I haven't fully thought through or tested the idea though, just mentioning as "maybe this is a scenario where you would set webViewKey but not keepWebViewInstanceAfterUnmount...."
There was a problem hiding this comment.
@TheAlmightyBob I think that would be a good use for the webViewKey. However, @donaldchen and I believe that for now it might just make sense to keep webViewKey alone and remove keepWebViewInstanceAfterUnmount. We can reintroduce that property at a later point when needed (perhaps, when supporting pop-up windows).
There was a problem hiding this comment.
It looks like Neal addressed this in 63c586a , so I'm going to resolve this comment thread.
EDIT: Actually, tentatively, I'm going to keep the comment thread unresolved, since links to comments (which I'm using for my own tracking) don't work as reliably for me in GitHub when conversations are hidden or resolved
There was a problem hiding this comment.
@donaldchen Er, does that mean you don't want me to resolve threads as I review changes...?
@cou929 , that's great that you got this working! Coincidentally, @nealmanaktola (my team member / coworker) got this working on Android in Discord's fork of @TheAlmightyBob , if we were to get this PR updated with both iOS and Android changes, and after this PR gets reviewed, would you be open to merging this PR? @Titozzz mentioned a new architecture for react-native-webview. Would we need to wait for those changes to complete before merging this PR?
I'll get back to you on that after I talk with my team and after @TheAlmightyBob responds to the question above. I'd like this PR, eventually with both iOS and Android changes, to get merged into react-native-webview/react-native-webview. |
@TheAlmightyBob, would you prefer a PR per platform (one for iOS and one for Android) or a single PR with both iOS and Android changes? |
@cou929 , for now, you can use Discord's fork of react-native-webview if you'd like to use these features on both iOS and Android https://github.com/discord/react-native-webview/commits/11.18.1-discord-1 . Later, depending on the response to #2469 (comment) , @nealmanaktola or I can take care of updating this PR, so we can hopefully get the iOS / Android changes merged into react-native-webview/react-native-webview. |
|
You can check my work for new arch the branch is published already and 95% of features are good |
Yes, but...
...I haven't been involved with this work and don't know the scope/timeline. I guess I'd encourage checking out his branch as he mentions above?
Although I expect it'll be very large, I think I'd lean towards a single PR. But if there's a logical way to organize the commits for reviewing incrementally, that might help. |
|
@Titozzz , do you have a rough estimate on when https://github.com/react-native-webview/react-native-webview/commits/new-arch will merge to master? @nealmanaktola and I will check out that branch and see if there are any major conflicts with out changes. Depending on when that new-arch branch is expected to merge, @nealmanaktola and I could either make our PR against
Sounds good! @nealmanaktola and I will initially plan on creating a single PR for both iOS and Android. |
Hey @TheAlmightyBob , thanks for the feedback! That makes sense. I'd like to update this PR to work with the new architecture, and I'll plan on adding more documentation, too. My availability is still TBD, but I'm hoping I can come back to this in one to two months. We'll tag you when this is ready for another review. Thanks! |
|
@donaldchen I have tried to use your branch, however the webview shows a blank page. It does not load for me. i have added <WebView
webViewKey="web1"
messagingWithWebViewKeyEnabled={true}
ref={webviewEl}My code also post messages const js = `
window.WebViewBridge = {
onMessage: (message) => {
const { type, data } = JSON.parse(message || '{}');
document.dispatchEvent(new CustomEvent(type, { detail: data }));
}
}any hint? |
|
@DavidGOrtega Does it work if you use it "normally" (without the webviewkey stuff)? I haven't used the branch with the latest changes, but a previous version was working for me. |
|
@DavidGOrtega , I haven't been available to test this branch recently, but I hope to test more when I get around to working on making this work with the new react native architecture #2469 (comment) . Like @TheAlmightyBob suggested, you could try an older commit such as 413cae3 . That one was before my last batch of changes in March. Also, what are you passing in for the
Can you share more context on where this code is running? Is this code that runs inside the WebView? You can view https://github.com/discord/react-native-webview/blob/237796aced1f986a5f8c33d5be295c5fbf74e728/example/examples/Portals.tsx#L215 for an example of posting messages to the WebView and https://github.com/discord/react-native-webview/blob/237796aced1f986a5f8c33d5be295c5fbf74e728/example/examples/Portals.tsx#L112 for an example of listening to messages sent from the WebView back to the react-native app that hosts the WebView. One difference I see is that your code uses |
Yes
Let me try it. The issue is more puzzling than I thought. Sometime loads the page, sometimes does not. For sure what is happening also is that If it loads and the device gets a locked screen if I go back the blank screen is there. This is tested in android 11 within a real device |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Small update from my end: I had begun updating this to support the new arch in discord#27 , but I haven't had time to get those changes fully working yet. I hope to revisit those changes again some time in the future |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
@TheAlmightyBob , I'd like to keep this PR open if possible. I'm still hoping to get back to this in the future, but I don't have a specific timeline yet |
i tried this PR, it crashes if keying it into a modal, or another screen, then unmounting that screen |
@Darren120 , this branch https://github.com/discord/react-native-webview/commits/11.18.1-discord-1 has a more stable version of this code. For the branch in this PR (https://github.com/discord/react-native-webview/tree/11.18.1-discord-2), I haven't had time to resolve all the errors and issues yet when making it work with the new react-native architecture |
UPDATE: |
An issue I am encountering is cookies are either not being shared if another webview sets them, or its being resetted |
further debugging, seems like the cookies are not being synced if a web view is activity being viewed while portaled. |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
FYI: I still like the idea behind that PR, I'm thinking of rewriting webview around new arch with this capability |
PR Status
Ready for review. We'd like this PR to get merged if it gets approved.
Overview of Changes
By default, moving a React
WebViewcomponent in the view hierarchy creates a new native WebView and refreshes the iframe. In Discord's mobile React Native apps which usereact-native-webview, we need the ability to move theWebViewwithout refreshing the iframe, and this PR adds support for that on iOS and Android by reusing the native WebView instance. We also need to be able to pass messages between the WebView and the react-native app without a react tag (e.g. when the React component for the WebView is not mounted), and this PR adds new APIs that support that, too. To test these changes, we added a "Portals" button to the RN-webview sample app. We've also been testing these changes in the Discord app.High level react-native-webview API changes
This PR updates react-native-webview with the following new APIs
webViewKeyreact prop- a key for accessing specific WebView instances. When this is defined, the native WebView stays in memory until explicitly released.releaseWebView- a function that explicitly releases native WebViews given a WebView key.messagingWithWebViewKeyEnabledreact prop - enables message passing without a react tag (i.e. without needing the react component to be mounted)injectJavaScriptWithWebViewKey- supports passing messages / injecting javascript to the native WebView given a WebView key even if the react component for the native WebView is not mounted anywhereaddOnMessageListenerWithWebViewKey- When thewebViewKeyis defined,onMessageand similar callbacks likeonLoadStarteddon't work. This method adds a listener for messages from the WebView given a WebView key, and this handles messages even when the react component is unmounted.How This Works
iOS Changes:
We have a singleton map where the keys are strings, and the values are
WKWebViews. When React Native asks for a new view, we create a newRNCWebViewinstance.RNCWebViewis a parent ofWKWebView, and when the props say to reuse the native WebView instance,RNCWebViewlooks for the existingWKWebViewinstance from the map before it decides whether it needs to create a newWKWebViewinstance. Additionally, when we move aWKWebViewbetween twoRNCWebViewparents, we need to remove the reference to the old parent that was held through the observer. We also have a map of web view keys toRNCWebViews, so we can remove the correct observer from theWKWebView. If we don't remove the observer when we change theWKWebView's parent, the app will crash with anEXC_BAD_ACCESSerror.Android Changes:
We renamed the old
RNCWebView, which extended the AndroidWebViewtoInternalWebView. Then we were able to wrapInternalWebViewwith a new view, which we namedRNCWebView, which is aFrameLayout. Now thatInternalWebViewis wrapped by theRNCWebViewview group, the Android architecture can match the iOS architecture where theRNCWebViewhas a one-to-one relationship with the react component, and the inner / native WebView does not have a one-to-one relationship with the react component. Similar to the iOS architecture, we also have singleton maps that hold references toRNCWebViews andInternalWebViews for WebView keys.Before and After
Here's the behavior where the iframe refreshes when
webViewKeyundefined.RN.WebView.before.mov
Here's the behavior where the iframe maintains its state as the react WebView moves in the view hierarchy when
webViewKeyis defined.Screen.Recording.2022-04-29.at.2.17.02.PM.mov
Testing messaging passing without a react tag
See discord#6 and discord#12 for screen recordings and explanations on how we tested the message passing between the WebView and the react-native app without a react tag.