Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
javache
left a comment
javache
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I think it would be good to split this in two changes: one that adds support to react/renderer/core and one that adopts in the Android integration.
| fun dispatchUnique(eventName: String, params: WritableMap?, eventTimestamp: Long) { | ||
| dispatchUnique(eventName, params, eventTimestamp, 0) | ||
| } |
There was a problem hiding this comment.
This class is internal - we don't need to do overloads or keep the API compatible, let's just make sure we update all the callers.
There was a problem hiding this comment.
Implemented in the now-published Android follow-up #59626 (fe48881): removed the compatibility overload from internal EventEmitterWrapper and updated every dispatchUnique caller, including pending view events and test expectations. The 17 event-ordering tests and 1 emitter-forwarding test pass on the rebased commit. Full JNI/device verification is still pending; the follow-up is draft and depends on the renderer-only revision in this PR. The earlier fork publishing blocker is resolved.
| // queue. | ||
| if (it->isUnique && it->type == rawEvent.type) { | ||
| if (it->isUnique && it->type == rawEvent.type && | ||
| it->coalescingKey == rawEvent.coalescingKey) { |
There was a problem hiding this comment.
Should this account for RawEvent::DefaultCoalescingKey?
There was a problem hiding this comment.
Published in the renderer-only revision bc06bf5. RawEvent::DefaultCoalescingKey is the ordinary key 0, not a wildcard: default/default events preserve existing coalescing behavior, while a default-key event and a different custom-key event remain separate. Added C++ cases for both plus interleaved-key ordering. Those renderer GTests have not been executed locally; native CI is pending. The 18 passing local Android tests apply to the separate dependent follow-up #59626, not these C++ tests.
|
The fork lock is cleared and the requested split is now published on current upstream main
Re-ran the Android follow-up suites: 17 event-ordering tests plus 1 emitter-forwarding test passed, 0 failures/errors. Changed Kotlin/C++ formatting and diff checks pass. C++ API snapshots were regenerated with CI-pinned Doxygen 1.16.1 before the clean rebase. Renderer GTests and full JNI/device verification have not been run locally; current native CI is pending. This renderer PR is ready for re-review. The Android draft will need rebasing after the renderer change is imported. |
4635137 to
bc06bf5
Compare
sorinc03
changed the title
|
@javache The requested split is now published: this PR is renderer-only at bc06bf5, and Android adoption is the dependent draft #59626. The internal overload and DefaultCoalescingKey replies are updated in their threads. This core PR is ready for re-review; native CI is running, and the description distinguishes the added-but-unrun renderer GTests from the 18 passing Android follow-up tests. |
Summary
Part 1 of the split requested in review; related to #54635. This PR now contains shared renderer support only, not Android adoption.
RawEventand require matching target, event type, and key before replacing an event.RawEvent::DefaultCoalescingKey(0). The default is an ordinary key, not a wildcard.Rebased onto upstream
9c7b0f5. Android integration is the separate dependent draft #59626; this core change alone does not fix the Android reporting path.Changelog
[INTERNAL] [CHANGED] - Add custom coalescing-key support to shared Fabric renderer events
Test Plan
git diff --check origin/main..HEAD: passed.node scripts/clang-format.js --check <changed C++ files>: passed.EventQueueCoalescingTestcases inEventQueueProcessorTest.cpp. These C++ GTests have not been executed locally; native CI verification is pending.The 18 passing Android integration tests belong to the separate Android follow-up, not this renderer-only revision.