Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] #15894
BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] #15894
Changes from 4 commits
0ecb4ee
df92f4a
c58ea62
a72e0f4
424e96a
a7d306c
5f998b2
17ecba0
4c6961e
7bf91d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add some nonnull attributes to prevent crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file could use
NS_ASSUME_NONNULL_BEGIN
, feel free to open a PR :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, that does not look right. Why do we need put tag inside body? That was needed for old poorly designed
sendEventBlahBlahWithoutTagButWithBody:
, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure JS needs the tag in the body that's why we put it here. I could be wrong though I'd have to look at the RCTEventEmitter.receiveEvent code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it is definitely used https://github.com/facebook/react/blob/7f78749ee3023608a614b2f595739bcd36acc128/src/renderers/native/ReactNativeEventEmitter.js#L112. I'd rather not touch this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... But... I am trying understand that code... and I failed.
Looks like we are sending
tag
asrootNodeID
(which is not even tag!) as a first argument... Why? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I'm not sure why we are sending the tag as both the first arg and in the event body as target. I just think we should keep it like this to avoid breaking react.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shergin Let's see if someone from the react team can help here. I agree we should have a clear plan here. We also have to consider android if we make any change to the native / react bridging api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I'm only somewhat familiar with our event code. (I don't think anyone on the core team presently is deeply familiar with it, to be honest.)
The event params like
target
seem to be optional given the Flow typing and the fact that we fallback to undefined values.If the 2 values are always the same (for Android too) then it seems silly to always pass them both and I could update the
ReactNativeEventEmitter
to just create this wrapper object (or not, if we determine it's not actually being used for anything).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW it looks like the Java implementation doesn't necessarily pass a
target
but it does pass other attributes in some cases (eg on content size change). The Java interface also defines the map as a@Nullable
type and in at least some cases explicitly passesnull
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bvaughn, I'm pretty sure we need to keep target in the event payload otherwise it would be a breaking change for apps that rely on this value. Maybe we could avoir passing the tag as the first argument if it's always the same as body.target. Needs more investigation.
@shergin What do you think about landing this as is since it will unblock a few other improvements I wanted to make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! To be clear: I wasn't suggesting we remove
event.target
on the JS side, just that we could remove the duplicate parameter and (on the RN JavaScript side) just always set target equal to the tag number.But that's obviously kind of a polish detail and could be done, or not, at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two usage of this in internal code base, which is cool and manageable!