Skip to content

iOS: Loop through RCTViewManager inheritance tree#14260

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.

Sign up for GitHub

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

Closed
cbrevik wants to merge 1 commit into from
Closed

iOS: Loop through RCTViewManager inheritance tree #14260

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 40 additions & 34 deletions React/Views/RCTComponentData.m
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -383,6 +383,7 @@ - (void)setProps:(NSDictionary<NSString *, id> *)props forShadowView:(RCTShadowV

- (NSDictionary<NSString *, id> *)viewConfig
{
NSMutableDictionary *propTypes = [NSMutableDictionary new];
NSMutableArray<NSString *> *bubblingEvents = [NSMutableArray new];
NSMutableArray<NSString *> *directEvents = [NSMutableArray new];

Expand All@@ -395,42 +396,47 @@ - (void)setProps:(NSDictionary<NSString *, id> *)props forShadowView:(RCTShadowV
}
}
#pragma clang diagnostic pop

unsigned int count = 0;
NSMutableDictionary *propTypes = [NSMutableDictionary new];
Method *methods = class_copyMethodList(object_getClass(_managerClass), &count);
for (unsigned int i = 0; i < count; i++) {
SEL selector = method_getName(methods[i]);
const char *selectorName = sel_getName(selector);
if (strncmp(selectorName, "propConfig", strlen("propConfig")) != 0) {
continue;
}

// We need to handle both propConfig_* and propConfigShadow_* methods
const char *underscorePos = strchr(selectorName + strlen("propConfig"), '_');
if (!underscorePos) {
continue;
}

NSString *name = @(underscorePos + 1);
NSString *type = ((NSArray<NSString *> *(*)(id, SEL))objc_msgSend)(_managerClass, selector)[0];
if (RCT_DEBUG && propTypes[name] && ![propTypes[name] isEqualToString:type]) {
RCTLogError(@"Property '%@' of component '%@' redefined from '%@' "
"to '%@'", name, _name, propTypes[name], type);
}

if ([type isEqualToString:@"RCTBubblingEventBlock"]) {
[bubblingEvents addObject:RCTNormalizeInputEventName(name)];
propTypes[name] = @"BOOL";
} else if ([type isEqualToString:@"RCTDirectEventBlock"]) {
[directEvents addObject:RCTNormalizeInputEventName(name)];
propTypes[name] = @"BOOL";
} else {
propTypes[name] = type;

Class superClass = _managerClass;
while (superClass && superClass != [RCTViewManager class]) {
if ([superClass isSubclassOfClass:[RCTViewManager class]]) {

@javachejavacheMay 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cbrevikcbrevikMay 31, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second part of the while condition excludes RCTViewManager.

superClass != [RCTViewManager class]

So it shouldn't add RCTViewManager props.

@cbrevikcbrevikMay 31, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sherginsherginMay 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@javachejavacheMay 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sherginsherginMay 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sherginsherginMay 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

@javachejavacheMay 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

unsigned int count = 0;
Method *methods = class_copyMethodList(object_getClass(superClass), &count);
for (unsigned int i = 0; i < count; i++) {
SEL selector = method_getName(methods[i]);
const char *selectorName = sel_getName(selector);
if (strncmp(selectorName, "propConfig", strlen("propConfig")) != 0) {
continue;
}

// We need to handle both propConfig_* and propConfigShadow_* methods
const char *underscorePos = strchr(selectorName + strlen("propConfig"), '_');
if (!underscorePos) {
continue;
}

NSString *name = @(underscorePos + 1);
NSString *type = ((NSArray<NSString *> *(*)(id, SEL))objc_msgSend)(superClass, selector)[0];
if (RCT_DEBUG && propTypes[name] && ![propTypes[name] isEqualToString:type]) {
RCTLogError(@"Property '%@' of component '%@' redefined from '%@' "
"to '%@'", name, _name, propTypes[name], type);
}

if ([type isEqualToString:@"RCTBubblingEventBlock"]) {
[bubblingEvents addObject:RCTNormalizeInputEventName(name)];
propTypes[name] = @"BOOL";
} else if ([type isEqualToString:@"RCTDirectEventBlock"]) {
[directEvents addObject:RCTNormalizeInputEventName(name)];
propTypes[name] = @"BOOL";
} else {
propTypes[name] = type;
}
}
free(methods);
}
superClass = [superClass superclass];
}
free(methods);


#if RCT_DEBUG
for (NSString *event in bubblingEvents) {
if ([directEvents containsObject:event]) {
Expand Down