[WebView] Add props for overriding native component

[cbrevik]

Motivation
Just like with #10105, the motivation is to make WebView more extensible.

In order to extend ReactWebViewManager (Android) with your own custom logic in a new class, you will have to override getName, and refer to this name in the JavaScript-code. And that's fine.

But, the problem is that you will have to duplicate the entire file for WebView.android.js in order to refer to the new custom native component by name with requireNativeComponent (since it is hard-coded to 'RCTWebView'). It's also necessary to do so in order to pass through any custom events or props you may want to set on the custom native WebView.

All of the above is also true for iOS.

What I'm trying to solve with this PR is give the ability to extend native WebView logic, while being able to re-use and extend existing WebView JavaScript-logic. This is done by adding a new nativeComponent prop on WebView. I've also added nativeComponentProps so custom props / events can be set on the custom native WebView.

Test plan

Let's say I've extended ReactWebViewManager with my own CustomWebViewManager in my Android-project (overriding getName so it returns "RCTCustomWebView"), and this new class may emit a onSomethingHappened event.
Then I can implement my custom WebView as follows in JavaScript:

import React, { Component, PropTypes } from 'react';
import {
    WebView
} from 'react-native';

import requireNativeComponent from 'requireNativeComponent';

class CustomWebView extends Component {
    constructor(props) {
        super(props);
    }

    static propTypes = {
        ...WebView.propTypes,
        onSomethingHappened: PropTypes.func,
    };

    onSomethingHappenedEvent = (event) => {
        var { onSomethingHappened } = this.props;
        onSomethingHappened && onSomethingHappened(event);
    }

    render() {
        return (
            <WebView 
              {...this.props} 
              nativeComponent={RCTCustomWebView} 
              nativeComponentProps={{ onSomethingHappened: this.onSomethingHappenedEvent }} 
            />
        );
    }
}

var RCTCustomWebView = requireNativeComponent('RCTCustomWebView', CustomWebView);

module.exports = CustomWebView;

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @jacobp100 and @spicyj to be potential reviewers.

[eslint-bot]

no-trailing-spaces: Trailing spaces not allowed.

[eslint-bot]

no-trailing-spaces: Trailing spaces not allowed.

[jacobp100]

This is a nice way to handle extending native components.

[cbrevik]

This could in principle be done with more React Native components, to make it easier for the community to re-use and extend native components.

[mkonicek]

Interesting approach to reusing the WebView.*.js. Have you tried it out on iOS by extending the WebView in Obj-C?

Why do you need to extend the Android WebView, however? If there are bugs or missing features would you be up for contributing them upstream?

I'm curious why we haven't had a use case like this with any of the components (Text, TextInput etc.) so far. If there are very few people who need to extend the native code a reasonable tradeoff might be for you to fork the WebView.*.js so we don't add two extra props to the public API very few people would use.

[cbrevik]

@mkonicek

I've tested on iOS as well, and this approach works there as well.

You'll have to duplicate RCTWebViewManager.m, since the exported macro-defined methods/properties (with RCT_REMAP_VIEW_PROPERTY, RCT_EXPORT_METHOD, etc) are only picked up from the subclass, and not from the parent. So you cannot inherit the props from RCTWebViewManager, which is a shame.
But, you will not have to duplicate RCTWebView.m, and can easily enough inherit and extend that class.

One issue I've found in WebView.ios.js is that the RCTWebViewManager is also referenced by name, and is used when deciding to startLoadWithResult or not. This could be remedied though if we did like with the rest of the exported methods from the RCTWebViewManager and call startLoadWithResult with dispatchViewManagerCommand.

I don't really see the need to reference the ViewManager directly, or maybe I am missing something?

---

Why not just contribute upstream?

There might be features which are a bit too specific to a single use-case, like with for example #9949.
In my specific case I have some sensitive logic for handling inactivity in the WebView, which doesn't seem to fit upstream either.

I don't think this is satisfactory resolved by just forking the logic. It is difficult to maintain in that case, and hard to keep up-to-date with each new RN version. Also the complexity of it is a bit too high for junior RN developers on my team, and I'd dread to hand off the project to someone else in that case.

See for example this discussion on extending core components. Or this issue with regards to duplication of code. Coincidentally both by @alinz so maybe he has some input here?

This might not be necessary for simple components (Text, TextInput, etc). But for more complex components, or those that need a lot of customization, this would help a lot I think.

[jacobp100]

@cbrevik Would you be able to look into what would need to happen to get extending the iOS webview to be as easy as Android’s (without manually copying over the props)? I ask so we don’t implement this solution, and then have to introduce breaking changes later to accommodate iOS.

[cbrevik]

Well I suspect the issue is with this line in RCTComponentData, where the class_copyMethodList method only retrieves the child methods.

We could potentially reflect (and maybe loop through) the inheritance tree with superclass, and retrieve the parents methods as well.

I'll test it later this evening.

[cbrevik]

@jacobp100

Just a preliminary test, but it seems like if we change that line to something like:

  Class managerClass = [_managerClass class];
  while (managerClass != [RCTViewManager class]) {
    Method *methods = class_copyMethodList(object_getClass(managerClass), &count);
    // snipped away props for-loop
    free(methods);
    managerClass = [managerClass superclass];
  }

Then it will loop through and add the parents' props as well, and then extending the ViewManager becomes very easy:

#import "RCTCustomWebViewManager.h"
#import "RCTCustomWebView.h"
#import "UIView+React.h"

@interface RCTCustomWebViewManager () <RCTWebViewDelegate>
@end

@implementation RCTCustomWebViewManager
{
}

RCT_EXPORT_MODULE()

- (UIView *)view
{
  RCTCustomWebView *webView = [RCTCustomWebView new];
  webView.delegate = self;
  return webView;
}

RCT_EXPORT_VIEW_PROPERTY(onSomethingHappened, RCTDirectEventBlock)

@end

[jacobp100]

Neat! Really excited about this.

I think we need some examples in UIExplorer and documentation—but I'm happy to help with that if you need!

[cbrevik]

Absolutely, help would be appreciated! Not sure if the RCTComponentData change belongs in its own PR though?

Or maybe not? I suppose both changes would be connected, we wouldn't be able to provide a good example in UIExplorer without landing this PR first.

[lacker]

Hey @jacobp100 are you still down to follow through on that generous offer to help out with the docs on this PR? ;-)

[jacobp100]

I could certainly do that! @cbrevik, the snippet where you copy over the superclass methods, where does that go?

[cbrevik]

@jacobp100 I can create a new PR for that snippet, it probably needs re-testing first since RCTComponentData has changed.

[jacobp100]

I know it's been some time—just wondering if you wanted to continue with this? I can do docs if you give me code examples! @cbrevik

[cbrevik]

@jacobp100 I've had this PR in the back of my mind. Code examples shouldn't venture too far from the original issue comment and the example there. I can take a look this week, and resolve conflicts etc.

[jacobp100]

@cbrevik I understood how to do it on the JS side, I wasn't able to get anything working on ObjC (and I didn't even try Java)

[cbrevik]

@jacobp100 Aha my bad, I'll try to put together an example repository for you. The Java-side is pretty trivial, Objective-C demands cloning the RCTWebViewManager as previously stated (until we have the ability to traverse the superclass inheritance tree).

[cbrevik]

@jacobp100 has written docs (ad98f85) for this new feature. So I think this PR is good to go for another review @lacker

[ptilli]

Great job guys!

I hope this feature would be extended to other components (especially Text ;;)) as well!!

[shergin]

@jacobp100 @cbrevik 👍 If you could present the changes in RCTComponentData.m (iOS) as separate PR, I will definitely review it and most likely merge. (Same probably true for Android.)
I would love to have this functionality at least in iOS but I have no time to tackle it myself.

[cbrevik]

@shergin Great! I'll open the two other PRs today 👍 This PR (and especially the docs) will be dependent on the other two PRs landing then.

[niztal]

Can someone merge it? I see all checks, tests and reviewes passed, all left is push the merge button.

It's blocking for other PRs to be merged.
#12807

[shergin]

Nope, it cannot be merged until we figure out (and implement) the right design of module inheritance. See discussion in #14260.
I plan to tackle this problem relatively soon (~this summer) anyway.
Or, @cbrevik, do you interested in implementing the approach roughtly descibed in #14260 (comment) ? 😄

[cbrevik]

Sure @shergin, something like this cbrevik@e71c4ed?

Bit unsure on how to re-establish inheritance with the base "ViewManager" name since NativeProps seems to be exported with the "View" name instead. (E.g. UIManager.RCTView.NativeProps.)

Maybe you know of a better way? Don't think looping through is the best approach.

Edit: Or, possibly need to loop through anyway if we want to support a longer module inheritance chain? RCTWebViewManager -> RCTViewManager -> RCTBaseViewManager?

[shergin]

@cbrevik We definitely want to support long inheritance chain.
I also think we have to avoid looping through UIManager; can we infer key from viewConfig.BaseManager?

[cbrevik]

@shergin Not sure, it's this bit of hacky code which determines the exported module name. Don't think we want to replicate that on the JS side.

I could extract that bit of code into its own method in RCTComponentData though, to also export the parent module name in the viewConfig. Then we could do something like UIManager[viewConfig.baseModuleName].

[shergin]

@cbrevik viewConfig.baseModuleName 👍

[cbrevik]

@shergin I've landed on an implementation which supports longer inheritance chains. I'll test more, then open a PR.

[shergin]

@cbrevik Omg, omg! That's awesome!!1 Please, go ahead and open PR (even if it is not finished, we will discuss approach there)!

Current implementation will break Android (and all other platforms), right? Can we easily implement baseModuleName for Android or we should temporary workaround it on JS side?

[cbrevik]

@shergin I can open a PR a bit later then.

I don't think it will break Android since that platform already supports inheritance (so you don't have to merge in anything). That might need a it of testing though to verify.

As far as other platforms, UWP etc. we will have to find out.

Edit: Might be mistaken about Android. Doing a diff between them it seems like you inherit most, but not all of the props. Question is if we should merge the non-inherited ones at all? If the underlying ViewManager does not export them itself, why should they be merged on the JS-side?

[cbrevik]

Now that #14775 is landed, we're getting closer to landing this one as well.

To make the API for this functionality a bit more general, I'm thinking of changing the signature from:

<WebView 
  nativeComponent={RCTCustomWebView} 
  nativeComponentProps={{ onSomethingHappened: this.onSomethingHappenedEvent }} 
/>

To:

<WebView 
  nativeConfig={{
    component: RCTCustomWebView,
    props: { onSomethingHappened: this.onSomethingHappenedEvent }
  }} 
/>

Makes the API a bit cleaner, and also easier to extend with more options. For example if you want to send in a custom ViewManager as well (which it seems like the iOS WebView might require):

<WebView 
  nativeConfig={{
    component: RCTCustomWebView,
    props: { onSomethingHappened: this.onSomethingHappenedEvent },
    viewManager: CustomWebViewManager
  }} 
/>

Shouldn't be much work to make this change. Thoughts?

[jacobp100]

The extensibility of the new way definitely looks like an improvement!

[cbrevik]

Opened a new PR #15016 for this feature, since the implementation has changed over time. Closing this one in favor of that.