Skip to content

[RT] Minor clean up #11122

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

Merged
sebmarkbage merged 2 commits into from
Oct 5, 2017
Merged

[RT] Minor clean up #11122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e00b26e
Rename rootNodeID -> tag
sebmarkbage Oct 5, 2017
5525793
Process RT props before forwarding them along
sebmarkbage Oct 5, 2017
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
6 changes: 2 additions & 4 deletions src/renderers/native-rt/ReactNativeRTEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,12 @@ var ReactNativeRTEventEmitter = {
* @param {object} nativeEventParam Object passed from native.
*/
receiveEvent: function(
rootNodeID: number,
tag: number,
topLevelType: string,
nativeEventParam: Object,
) {
var nativeEvent = nativeEventParam;
var props = ReactNativeRTComponentTree.getFiberCurrentPropsFromTag(
rootNodeID,
);
var props = ReactNativeRTComponentTree.getFiberCurrentPropsFromTag(tag);
if (props == null) {
return;
}
Expand Down
24 changes: 22 additions & 2 deletions src/renderers/native-rt/ReactNativeRTFiberRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,26 @@ export type TextInstance = number;

const {precacheFiberNode, updateFiberProps} = ReactNativeRTComponentTree;

function processProps(instance: number, props: Props): Object {
const propsPayload = {};
for (var key in props) {
if (key === 'children') {
// Skip special case.
continue;
}
var value = props[key];
if (typeof value === 'function') {
value = {
style: 'rt-event',
event: key,
Copy link
Contributor

@bvaughn bvaughn Oct 5, 2017

Choose a reason for hiding this comment

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

nit: Might it be confusing that event ends up holding values like onTopChange instead of topChange? Maybe we should name it key or something instead?

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Oct 5, 2017

Choose a reason for hiding this comment

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

Given that this bypasses the event system it won't use the topChange mechanism. So not sure if it's confusing if you never new about the previous event system.

Copy link
Contributor

@bvaughn bvaughn Oct 5, 2017

Choose a reason for hiding this comment

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

Sure. Maybe that was just a poor example. My point was that the prop (key) usually corresponds to the event handler name rather than the event name. But maybe this new renderer is different?

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Oct 5, 2017

Choose a reason for hiding this comment

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

Right. This name is basically only here so that they can name the event name the same as the handler, so that we don't need any mapping logic on our side.

This is just one possible approach though.

Copy link
Contributor

@bvaughn bvaughn Oct 5, 2017

Choose a reason for hiding this comment

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

Okay. Sounds like you've thought about this and it's intentional then so...cool. 😄 Thanks for elaborating.

tag: instance,
};
}
propsPayload[key] = value;
}
return propsPayload;
}

const NativeRTRenderer = ReactFiberReconciler({
appendChild(parentInstance: Instance, child: Instance | TextInstance): void {
RTManager.appendChild(parentInstance, child);
Expand Down Expand Up @@ -70,7 +90,7 @@ const NativeRTRenderer = ReactFiberReconciler({
internalInstanceHandle: Object,
): void {
updateFiberProps(instance, newProps);
RTManager.updateNode(instance, newProps);
RTManager.updateNode(instance, processProps(instance, newProps));
},

createInstance(
Expand All @@ -81,9 +101,9 @@ const NativeRTRenderer = ReactFiberReconciler({
internalInstanceHandle: Object,
): Instance {
const tag = ReactNativeRTTagHandles.allocateTag();
RTManager.createNode(tag, type, props);
precacheFiberNode(internalInstanceHandle, tag);
updateFiberProps(tag, props);
RTManager.createNode(tag, type, processProps(tag, props));
return tag;
},

Expand Down