Skip to content

Fixing incorrect documentation tags#10530

Merged
flarnie merged 1 commit intofacebook:masterfrom
mitermayer:fixing-incorrect-documentation-tags
Aug 26, 2017
Merged

Fixing incorrect documentation tags#10530
flarnie merged 1 commit intofacebook:masterfrom
mitermayer:fixing-incorrect-documentation-tags

Conversation

@mitermayer
Copy link
Contributor

Minor nits, fixing the documentation tags params based on the flow type annotation

* @param {rootNodeID} rootNodeID React root node ID that event occurred on.
* @param {TopLevelType} topLevelType Top level type of event.
* @param {object} nativeEventParam Object passed from native.
* @param {?object} nativeEventParam Object passed from native.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering - how do we know that this third param is optional? Seems like it always gets passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Flarnie,

This one I could be wrong about it, I assumed it could be nullable due to the bellow line

var nativeEvent = nativeEventParam || EMPTY_NATIVE_EVENT;

Copy link
Contributor

Choose a reason for hiding this comment

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

That check does imply that - thinking we could update the Flow type here too in that case. Could be a follow-up PR.

@flarnie flarnie self-requested a review August 25, 2017 16:22
Copy link
Contributor

@flarnie flarnie left a comment

Choose a reason for hiding this comment

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

Thanks!

@flarnie flarnie merged commit a7c479b into facebook:master Aug 26, 2017
@gaearon
Copy link
Collaborator

gaearon commented Aug 26, 2017

In the future I’d prefer that we remove jsdocs for arguments. Types are often wrong or outdated (since, unlike Flow, they’re not checked), and names should be descriptive anyway.

@mitermayer
Copy link
Contributor Author

@gaearon We need to chat, the reason I managed to find all this documentations miss places is because I been working on a solution that solves exactly that matter that you described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants