iOS: Loop through RCTViewManager inheritance tree

[cbrevik]

Motivation

See discussion in #10946. The motivation is to make RCTViewManager more extensible.

If you want to inherit from a native ViewManager, your custom ViewManager will not automatically expose 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 automatically looping through the parents' props, and exposing those for the child ViewManager as well.

Test plan
The test plan is the same as the other PR (#10946). I've made an simple test app which extends RCTWebViewManager: https://github.com/cbrevik/overrideWebview

See RCTCustomWebViewManager.h and RCTCustomWebViewManager.m for a simple implementation.

CC @shergin (#10946 (comment))

[javache]

While this is a good idea, UIManager constants is already one of the more expensive parts of bridge startup, and this increases it significantly, since every view manager inherits from RCTViewManager.

Could you consider different approach, e.g. by sending the hierarchy info over to JS instead, and dynamically figuring out if the supported prop is in the inheritance chain.

We should also decide whether everything inheriting from RCTViewManager makes sense. I'm sure there's plenty of views that don't want to inherit all of these props, and this doesn't give them a choice.

[shergin]

@javache Yeah, Pieter, performance is also my biggest concern here.
I have not reviewed this diff carefully yet, but seems that we add only one additional call (message pass) to view managers which do not utilize new (inheritance) feature, and it is superClass = [superClass superclass]; (we probably can replace it with one magic ObjC runtime C function). Right?

I am assuming that actual base class (RCTViewManager, which contains a ton of props) is parsed once somewhere else.

[cbrevik]

@shergin Yes, exactly. The idea was that the performance footprint should stay relatively similar for existing modules. Only those that does not inherit directly from RCTViewManager should do more than one loop.

E.g. from my example app, the RCTCustomWebViewManager which inherits from RCTWebViewManager will do two loops, one to copy props from RCTCustomWebViewManager, and another to copy from RCTWebViewManager.

@javache Have to admit that I haven't run a perfomance timer on this code. I've only tried to compare startup time of RNTester with and without this change. Didn't see a difference in startup time there, but it would probably be worth it to do a more precise comparison.

[javache]

All ViewManagers right now inherit from RCTViewManager, so they will also be inheritings its massive list of props (some of those won't even works, since they only apply to RCTView, and not the UIView-subclasses other view managers may vend).

I would suggest splitting RCTViewManager into a RCTViewManagerBase (abstract superclass) and RCTViewManager, and have most view managers inherit from RCTViewManagerBase.

[javache]

Also, let's make sure this behaviour is consistent with the Android API's.

[shergin]

@javache
Yes, yeah, I agree it would be awesome if we can introduce some another base class for managers which will contain only completely necessary props, and RCTViewManager will have all our sophisticated styling props. If we do this, we can also get rid of all kinda default implementation of these style props inside RCTViewManager (because we can assume that all views generated by
RCTViewManager are RCTView subclasses). (This is my everyday pain and my dream. I am already working on making (at least) and RCTView subclasses.)

So... It actually means that we NEED this PR for allowing inheritance! Right?

Speaking about performance,
Yes, all view managers inherit from RCTViewManager right now, but we do not recreate/regenerate view configs for/from RCTViewManager for every subclass. We did not do this, and this PR do not introduce it. I also believe we have to cache viewConfigs for every class though. (Not in this PR.)

So, I propose we should make it real step by step:

1. Introduce the possibility of inheritance (this PR);
2. Add cache mechanism for all view manager classes. (We can store generated config as class/static variable.) (It will help a lot for next step!)
3. Introduce a new view manager base class (something like RCTElementManager) and move bunch of core props from RCTViewManager to this class.
4. Introduce a new "policy": new RCTViewManager subclasses should not rely on "default" props implementation inside RCTViewManager. We can enforce via warning;
5. Make all really custom elements (such as NavigationBar or something like that) inherit from RCTElementManager instead of RCTViewManager.
6. ...
7. Profit!

So, why I am personally interested in it? Because I would love to have base manager class for our both implementations.

[javache]

I'm not sure I understand @shergin. This code will iterate through all of the superclasses of a view manager (up-to-and-including RCTViewManager), so propTypes will be much bigger, and we'll have to send way more information to JS

[cbrevik]

The second part of the while condition excludes RCTViewManager.

superClass != [RCTViewManager class]

So it shouldn't add RCTViewManager props.

[cbrevik]

So it bascially loops through the inheritance-chain, until it hits RCTViewManager, and then breaks out. Which should make it behave as it does today, only that it supports further sub-classing of other view managers.

[shergin]

Yeah, it worked before even without iterating over RCTViewManager! So, even it it does now, it is not intentional. I would try to optimize this iteration a bit by using some ObjC runtime functions (because we can and because we care 😄 ), but I want to have conceptual consensus before.

[javache]

So, how about RCTViewManager itself then? Where does that get exported?

[shergin]

That the point. I have not idea, yet. 😄 Do you?
Okay, I will figure out. You are right, we have to know that before making this kind of changes.

[shergin]

(Just to be clear, I am no so reckless, I plan to investigate it before landing this, I just want to have a consensus among us related to the general move.)

[javache]

That's my point. This diff is broken for the base case of RCTViewManager as far as I can tell.

Other than that, I agree with your strategy (save for some bikeshedding on naming), atlhough I'd change the order of operations a bit: there should be no in-between state where RCTViewManager works but warned against, since it's not supported right now and getting rid of the warning is just going to be painful (your point 4)

[shergin]

@javache You are right. We have to rethink whole approach.

react-native/Libraries/ReactNative/requireNativeComponent.js

Lines 73 to 79 in 1129c60

	// The ViewConfig doesn't contain any props inherited from the view manager's
	// superclass, so we manually merge in the RCTView ones. Other inheritance
	// patterns are currenty not supported.
	const nativeProps = {
	...UIManager.RCTView.NativeProps,
	...viewConfig.NativeProps,
	};

The right approach can be:

• Export intermediate view managers as well;
• Add baseClassName info to viewConfig;
• And then re-establish inheritance relationship on JS side inside requireNativeComponent.

I can try to do it.

[cbrevik]

Ah sorry, I understand the issue now. In that case, your approach seems better @shergin and I'll close this PR in favor of that.