Skip to content

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 21, 2017

This is an expensive no-op for Android, and causes an unnecessary view invalidation for certain components (eg RCTTextInput) on iOS.

This was a perf regression with RN fiber. RN stack already handled this case.

This is an expensive no-op for Android, and causes an unnecessary view invalidation for certain components (eg RCTTextInput) on iOS.
Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

Mind adding a short test? Can be as simple as this.

expect(UIManager.updateView).not.toBeCalled();

Overall I like @emilsjolander's idea of doing a snapshot test too if we can figure out what is a realistic situation to capture.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 22, 2017

Good call. I added a couple of tests for this. I didn't cover NativeMethodsMixin because we don't seem to have any createReactClass specific tests and I was on the fence about adding one. I could go either way.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

tests lgtm, thanks!

@bvaughn bvaughn merged commit 6ab2869 into facebook:master Aug 22, 2017
@bvaughn bvaughn deleted the rn-bridge-noop branch August 22, 2017 20:31
@bvaughn bvaughn mentioned this pull request Sep 7, 2017
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