fix(image): emit onLoadStart before subscribing the response observer on Fabric - #58314
fix(image): emit onLoadStart before subscribing the response observer on Fabric#58314SatyamBansal wants to merge 1 commit into
Conversation
On Fabric, updateState: subscribed the image response observer before it emitted onLoadStart. ImageResponseObserverCoordinator::addObserver replays an already-Completed (or Failed) response synchronously, and RCTExecuteOnMainQueue runs the block inline when already on the main queue (which mounting is) - so a request that finished before the mount transaction applied delivered onLoad and onLoadEnd ahead of onLoadStart. Moving the emission above the subscribe call restores the ordering the old architecture guarantees (RCTImageView.reloadImage emits _onLoadStart before calling the loader). Both state locals are captured before the subscribe call, so the condition is unaffected by the move.
|
Hi @SatyamBansal! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Summary
Fixes #54120.
On the New Architecture on iOS, changing the
sourceof an already-mounted<Image>delivers the load events out of order:RCTImageComponentView.updateState:oldState:subscribes the image response observer before it emitsonLoadStart().ImageResponseObserverCoordinator::addObserverdoes not queue a finished request — it replays it synchronously (case Completed:→observer->didReceiveImage(...), andcase Failed:→didReceiveFailure(...)).RCTImageResponseObserverProxyforwards throughRCTExecuteOnMainQueue, which isif (RCTIsMainQueue()) { block(); }— inline, and mounting is on the main thread. So when a request has already completed by the time the mount transaction applies,didReceiveImage:emitsonLoad+onLoadEndfrom inside the subscribe call, andonLoadStartfollows afterwards.This moves the
onLoadStart()emission above the subscribe call, restoring the ordering the old architecture guarantees —RCTImageView.reloadImageemits_onLoadStartbefore it callsloadImageWithURLRequest:, so it cannot invert. Android is likewise unaffected:ReactImageViewbindsonLoadStartto Fresco'sonSubmitandonLoad/onLoadEndtoonFinalImageSet, and submission always precedes delivery.The move is safe with respect to the guard condition: both
oldImageStateandnewImageStateare captured from_state/statebefore the subscribe call, so hoisting the emission above it does not change what the condition sees.Why it matters
Any component that shows a spinner on
onLoadStartand hides it ononLoadEndis left with a permanently visible spinner over a fully decoded image — the event that turns it on arrives after the event that would turn it off. #54120 has reports of this from three separate users; the only workaround in the thread is a guessedsetTimeout.Changelog:
[IOS] [FIXED] - Emit
Image'sonLoadStartbeforeonLoad/onLoadEndwhen the image request has already completedTest Plan
Reproducer (community template, only
App.tsxdiffers): https://github.com/SatyamBansal/rn-repro-image-onloadstart-orderIt mounts four
<Image>s and records the order ofonLoadStart/onLoad/onLoadEndusing a counter incremented inside each callback (so the log is the order events reached JS, not a render artifact), withperformance.now()timestamps. Pressing Swap E source changes one mounted<Image>'ssourcebetween twodata:URIs.Verified on an iPhone 17 Pro simulator, iOS 26.3, New Architecture, debug,
react-native0.87.1 built from source (RCT_USE_PREBUILT_RNCORE=0, so this file is actually compiled — with the default prebuilt core a local edit here has no effect).Before — swap E's source:
Reproducible on every press. All three events land within 0.2 ms, inside one mount transaction — not an asynchronous race.
After this patch, same build, same gesture:
First-mount ordering is unchanged (it was already correct — on a fresh mount the request is still
Loadingwhen the view subscribes, becauseRCTImageManager.requestImagedispatches the fetch to a background queue after returning).Not covered by this test plan, and not run: Android and the iOS old architecture. Neither goes through this file, and both are argued above from source rather than measured.