Support native ViewManager inheritance on iOS #14775

cbrevik · 2017-06-29T10:54:20Z

Motivation
This is a re-worked version of #14260, by @shergin's suggestion.

For iOS, if you want to inherit from a native ViewManagers, your custom ViewManager will not automatically export the parents' props. So the only way to do this today, is to basically copy/paste the parent ViewManager-file, and add your own custom logic.

With this PR, this is made more extensible by exporting the baseModuleName (i.e. the iOS superclass of the ViewManager), and then using that value to re-establish the inheritance relationship in requireNativeComponent.

Test plan
I've run this with a test project, and it works fine there. But needs more testing.

Opened this PR as per @shergin's suggestion though, so we can discuss approach.

Discussion

Android already supports inheritance, so this change should be compatible with that. But, not every prop available on UIManager.RCTView.NativeProps is actually exported by every ViewManager. So should UIManager.RCTView.NativeProps still be merged with viewConfig.NativeProps, even if the individual ViewManager does not export/use them to begin with?
Does this break other platforms? UWP?

And re-establish inheritance relationship in requireNativeComponent

shergin

So, Android does not expose baseModuleName in viewConfig, right? So, this code will break it, right?

@javache What do you think?

shergin · 2017-06-29T18:46:17Z

React/Views/RCTComponentData.m

  return @{
    @"propTypes": propTypes,
    @"directEvents": directEvents,
    @"bubblingEvents": bubblingEvents,
+    @"baseModuleName": superClass != [NSObject class] ? [self moduleNameForClass:superClass] : [NSNull null]


nit: Please avoid "negative logic": Changing != to == will make this code easier to read.

shergin · 2017-06-29T18:48:58Z

Libraries/ReactNative/requireNativeComponent.js

-    ...UIManager.RCTView.NativeProps,
-    ...viewConfig.NativeProps,
-  };
+  let baseModuleName = viewConfig.baseModuleName;


Does using do { } while (); loop make this code a bit cleaner/easier-to-read?

do { } while (); would make it execute the statement at least once, which we don't want to do if there is no baseModuleName exported. So not sure if that is a good idea.

cbrevik · 2017-06-29T19:55:02Z

@shergin Android does not expose a baseModuleName, but since it already supports inheritance this code will not directly break because of that. That is to say, Android already exports most of the props through viewConfig.NativeProps, without having to merge with UIManager.RCTView.NativeProps. And since it does not export baseModuleName it will just skip the while() loop, because it has nothing it needs to merge.

The question is of course if it should still merge with UIManager.RCTView.NativeProps, when each ViewManager does not explicitly implement/export all the ones available on RCTView. If we still need to do that, then this code breaks Android at the moment.

shergin · 2017-06-30T00:18:09Z

It does support but how? Does Android use the same approach which we reject for iOS?
Okay, but current/old code does merge UIManager.RCTView.NativeProps and viewConfig.NativeProps for Android as well, right (line 76)? And seems the new code breaks it. No?

cbrevik · 2017-06-30T06:13:45Z

Yeah, the current/old logic does merge for Android as well. My question is if it should do that though?

For Android, using JSONDiff to compare what
viewConfig.NativeProps for AndroidTextInput contains vs. Androids UIManager.RCTView.NativeProps. There you see it is missing a few props from RCTView on AndroidTextInput, but most are already there. It seems like the inheritance chain on the native side (Java) already exports the inherited props (from parent ViewManagers) to viewConfig.NativeProps.

Whilst, for iOS, if you compare viewConfig.NativeProps for RCTTextView vs. iOS' UIManager.RCTView.NativeProps. There none of the props from RCTView.NativeProps already exists on viewConfig.NativeProps, so we have to merge the two.

So for Android, AndroidTextInput is missing a few from RCTView. But should we still merge the two if neither AndroidTextInput's ViewManager nor its parents implements the missing props on the native side?

cbrevik · 2017-06-30T06:25:20Z

For example the accessible prop is on RCTView (NativeProps), but is missing from AndroidTextInput (NativeProps). That prop is implemented on ReactViewManager.java.

But ReactTextInputManager.java does not extend ReactViewManager. So should accessible still be merged onto AndroidTextInput's NativeProps?

I grant that I might have misunderstood the logic here. 🤔

shergin · 2017-06-30T18:57:53Z

In this case, I think we should do this:

Split the logic here between iOS and other platforms. (And implement new logic only for iOS.)
Land it. And continue investigate benefits from this solution on iOS.
(In separate diff/PR.) We have to try remove merging only for Android (another split). And see will it break something or not and how it will affect performance.
Try to implement same (as on iOS) logic on Android (baseClassName and co). And see how it will affect performance. (It should improve TTI because it should significantly reduce amount of transferred data across the bridge. @javache Pieter, what do you think?)
Encourage WP/VR guys to do same thing for another platforms.
Celebrate wins. 🎉

@javache We need your opinion and blessing here. :)

javache · 2017-07-06T14:09:43Z

React/Views/RCTComponentData.m

@@ -455,4 +442,26 @@ - (RCTViewManagerUIBlock)uiBlockToAmendWithShadowViewRegistry:(NSDictionary<NSNu
  return nil;
 }

+- (NSString *)moduleNameForClass:(Class)managerClass


This method doesn't depend on any state, make it a static C-function.

javache · 2017-07-06T14:10:23Z

React/Views/RCTComponentData.m

  return @{
    @"propTypes": propTypes,
    @"directEvents": directEvents,
    @"bubblingEvents": bubblingEvents,
+    @"baseModuleName": superClass != [NSObject class] ? [self moduleNameForClass:superClass] : [NSNull null]


Use (id)kCFNull instead of [NSNull null]

javache · 2017-07-06T14:12:30Z

Code looks good to me, thanks for providing those JSON diffs! I don't understand yet why Android behaves differently here, we should probably understand that before we land this.

facebook-github-bot · 2017-07-10T21:31:01Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Opening a new PR for #10946 (see discussion there). This PR builds upon #14775 (iOS ViewManager inheritance) and #14261 (more extensible Android WebView). **Motivation** When `WebView.android.js` and `WebView.ios.js` use `requireNativeComponent`, they are hard-coded to require `RCTWebView`. This means if you want to re-use the same JS-logic, but require a custom native WebView-implementation, you have to duplicate the entire JS-code files. The same is true if you want to pass through any custom events or props, which you want to set on the custom native `WebView`. What I'm trying to solve with this PR is to able to extend native WebView logic, and being able to re-use and extend existing WebView JS-logic. This is done by adding a new `nativeConfig` prop on WebView. I've also moved the extra `requireNativeComponent` config to `WebView.extraNativeComponentConfig` for easier re-use. **Test plan** jacobp100 has been kind enough to help me with docs for this new feature. So that is part of the PR and can be read for some information. I've also created an example app which demonstrates how to use this functionality: https://github.com/cbrevik/webview-native-config-example If you've implemented the native side as in the example repo above, it should be fairly easy to use from JavaScript like this: ```javascript import React, { Component, PropTypes } from 'react'; import { WebView, requireNativeComponent, NativeModules } from 'react-native'; const { CustomWebViewManager } = NativeModules; export default class CustomWebView extends Component { static propTypes = { ...WebView.propTypes, finalUrl: PropTypes.string, onNavigationCompleted: PropTypes.func, }; _onNavigationCompleted = (event) => { const { onNavigationCompleted } = this.props; onNavigationCompleted && onNavigationCompleted(event); } render() { return ( <WebView {...this.props} nativeConfig={{ component: RCTCustomWebView, props: { finalUrl: this.props.finalUrl, onNavigationCompleted: this._onNavigationCompleted, }, viewManager: CustomWebViewManager }} /> ); } } const RCTCustomWebView = requireNativeComponent( 'RCTCustomWebView', CustomWebView, WebView.extraNativeComponentConfig ); ``` As you see, you require the custom native implementation at the bottom, and send in that along with any custom props with the `nativeConfig` prop on the `WebView`. You also send in the `viewManager` since iOS requires that for `startLoadWithResult`. **Discussion** As noted in the original PR, 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. Closes #15016 Differential Revision: D5701280 Pulled By: hramos fbshipit-source-id: 6c3702654339b037ee81d190c623b8857550e972

Add baseModuleName module constant for inheritance

3d9b553

And re-establish inheritance relationship in requireNativeComponent

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jun 29, 2017

shergin reviewed Jun 29, 2017

View reviewed changes

shergin requested a review from javache June 30, 2017 18:58

javache reviewed Jul 6, 2017

View reviewed changes

cbrevik and others added 2 commits July 6, 2017 18:28

Use kCFNull & static c function

c011d5e

Update RCTComponentData.m

4f7bff2

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 10, 2017

facebook-github-bot closed this in 684e035 Jul 10, 2017

This was referenced Jul 12, 2017

[WebView] Add props for overriding native component #10946

Closed

[WebView] Add props for overriding native component #15016

Closed

cbrevik mentioned this pull request Nov 26, 2017

Unsupported top level event type "topLoadingStart" dispatched in Custom WebView #16848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support native ViewManager inheritance on iOS #14775

Support native ViewManager inheritance on iOS #14775

cbrevik commented Jun 29, 2017

shergin left a comment

shergin Jun 29, 2017

shergin Jun 29, 2017

cbrevik Jun 29, 2017

cbrevik commented Jun 29, 2017 •

edited

Loading

shergin commented Jun 30, 2017

cbrevik commented Jun 30, 2017 •

edited

Loading

cbrevik commented Jun 30, 2017 •

edited

Loading

shergin commented Jun 30, 2017

javache Jul 6, 2017

javache Jul 6, 2017

javache commented Jul 6, 2017

facebook-github-bot commented Jul 10, 2017

Support native ViewManager inheritance on iOS #14775

Support native ViewManager inheritance on iOS #14775

Conversation

cbrevik commented Jun 29, 2017

shergin left a comment

Choose a reason for hiding this comment

shergin Jun 29, 2017

Choose a reason for hiding this comment

shergin Jun 29, 2017

Choose a reason for hiding this comment

cbrevik Jun 29, 2017

Choose a reason for hiding this comment

cbrevik commented Jun 29, 2017 • edited Loading

shergin commented Jun 30, 2017

cbrevik commented Jun 30, 2017 • edited Loading

cbrevik commented Jun 30, 2017 • edited Loading

shergin commented Jun 30, 2017

javache Jul 6, 2017

Choose a reason for hiding this comment

javache Jul 6, 2017

Choose a reason for hiding this comment

javache commented Jul 6, 2017

facebook-github-bot commented Jul 10, 2017

cbrevik commented Jun 29, 2017 •

edited

Loading

cbrevik commented Jun 30, 2017 •

edited

Loading

cbrevik commented Jun 30, 2017 •

edited

Loading