Skip to content

RN Inspector guard against clicks outside of RN#9911

Merged
bvaughn merged 1 commit intofacebook:masterfrom
bvaughn:rn-inspector-bugfix
Jun 9, 2017
Merged

RN Inspector guard against clicks outside of RN#9911
bvaughn merged 1 commit intofacebook:masterfrom
bvaughn:rn-inspector-bugfix

Conversation

@bvaughn
Copy link
Copy Markdown
Contributor

@bvaughn bvaughn commented Jun 9, 2017

Clicking "outside" of a React Native app can cause the latest incarnation of the Inspector to choke. This diff adds guards for this case.

measure: callback =>
UIManager.measure(component.getHostNode(), callback),
props: getHostProps(component),
source: component._currentElement && component._currentElement._source,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need this? It’s only called for items in the hierarchy array which I presume are already not nulls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I only added enough things to prevent the repro steps from crashing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This means we’re doing something else that’s bad. How do nulls end up in owner hierarchy?

UIManager.measure(component.getHostNode(), callback),
props: getHostProps(component),
source: component._currentElement && component._currentElement._source,
source: component &&
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would probably prefer an early return of “empty hierarchy” rather than these kinds of checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Like you do in Fiber.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup! Testing that now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, that's nicer. Thanks for pointing out the obvious. 😄

// Handle case where user clicks outside of ReactNative
if (!closestInstance) {
return {
hierarchy: emptyObject,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hierarchy is an array.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Schwoops.

@bvaughn bvaughn force-pushed the rn-inspector-bugfix branch from e392d6c to c892d86 Compare June 9, 2017 21:57
const instance = lastNotNativeInstance(componentHierarchy);

// Handle case where user clicks outside of ReactNative
if (!instance) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we exit earlier, if component is null?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(which is equivalent to closestInstance in the other version)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would be better. 😄

@bvaughn bvaughn force-pushed the rn-inspector-bugfix branch from c892d86 to 6bf5c50 Compare June 9, 2017 22:29
@bvaughn bvaughn merged commit 4aea7c3 into facebook:master Jun 9, 2017
@bvaughn bvaughn deleted the rn-inspector-bugfix branch June 9, 2017 23:10
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.

3 participants