fix(types): declare progress/uploadProgress callback params as string - #484
Open
giaBaoJS wants to merge 1 commit into
Open
fix(types): declare progress/uploadProgress callback params as string#484giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
Both the download and upload progress events carry `written` and `total` as strings on iOS and Android, but the type declarations claimed `number` in three of the four overloads. Only `progress(callback)` was correct, which made the two `progress()` overloads contradict each other even though they are fed by the same event. Native emission sites, all of which stringify: android .../ReactNativeBlobUtilReq.java:225-226 android .../Response/ReactNativeBlobUtilDefaultResp.java:73-74 android .../Response/ReactNativeBlobUtilFileResp.java:170-171 android .../ReactNativeBlobUtilBody.java:441-442 ios ReactNativeBlobUtilRequest.mm:428-429,436-437,478-479,563-564,655-656 Align the remaining overloads in index.d.ts and both callback aliases in index.js.flow with that behaviour, and update the README examples so the documented arithmetic converts explicitly. This is a type-only change; no runtime behaviour is affected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #410.
The
progress()overloads contradict each other:progress(callback)declares(received: string, total: string)whileprogress(config, callback)declares(received: number, total: number)— even though both are fed by the sameReactNativeBlobUtilProgressevent. The declaration that saysnumberis the wrong one; native stringifies on both platforms.You mentioned in #410 that the types would have to be cast, so here is that change.
Native emission sites
Every site that emits
written/totalon iOS and Android stringifies:ReactNativeBlobUtilReq.java:225-226EVENT_PROGRESSputString(String.valueOf(...))Response/ReactNativeBlobUtilDefaultResp.java:73-74EVENT_PROGRESSputString(String.valueOf(...))Response/ReactNativeBlobUtilFileResp.java:170-171EVENT_PROGRESSputString(String.valueOf(...))ReactNativeBlobUtilBody.java:441-442EVENT_UPLOAD_PROGRESSputString(String.valueOf(...))ReactNativeBlobUtilRequest.mm:428-429,436-437EVENT_PROGRESS[NSString stringWithFormat:]ReactNativeBlobUtilRequest.mm:478-479EVENT_PROGRESS[NSString stringWithFormat:]ReactNativeBlobUtilRequest.mm:655-656EVENT_PROGRESS[NSString stringWithFormat:]ReactNativeBlobUtilRequest.mm:563-564EVENT_PROGRESS_UPLOAD[NSString stringWithFormat:]fetch.js:195andfetch.js:202pass these straight through to the user callback, so nothing converts them on the way.What changed
index.d.ts—progress(config, callback)and bothuploadProgressoverloads now declarestring.index.js.flow—ProgressCallbackandUploadProgressCallbacklikewise. Without this the two type systems shipped in the package would disagree with each other.README.md— the four progress examples usedreceived / total, which is fine at runtime but stops compiling under the corrected types. They now convert explicitly, which doubles as the migration hint.Why
uploadProgressis includeduploadProgressdeclaresnumberin both overloads and is wrong in both — it reads the samewritten/totalfields, emitted as strings fromReactNativeBlobUtilBody.java:441-442andReactNativeBlobUtilRequest.mm:563-564. Fixing onlyprogress()would leave the sibling API telling the same lie. Happy to split it into its own PR if you would rather keep this scoped to #410.The direction question
There are two ways to close this, and they break different people:
string— what this PR does. Compile-time break only, for TypeScript/Flow users doing arithmetic on the params. Zero runtime change; existing JS keeps working exactly as before, sincereceived / totalcoerces.received === '100',parseInt(received), string concatenation, etc., across four Android sites and five iOS sites.I went with (a) because it matches your "would have to cast the types" comment, because it is what #410 asks for, and because it makes the declaration honest without touching runtime behaviour. If you would rather have (b), say the word and I will redo this as the native change instead.
Worth flagging: #300 asks for the opposite of this PR. It requests
string→numberso thatMath.floor((received/total) * 100)type-checks. That request is only satisfiable via (b), so I have deliberately not marked #300 as fixed here — under (a) the answer to #300 becomes "convert at the call site". Your call which way that lands.Windows diverges — worth a separate look
windows/ReactNativeBlobUtil/ReactNativeBlobUtil.cpp:1806-1807emitswrittenasint64_tandtotalas aUInt64ornull— i.e. numbers, not strings. So Windows is genuinely inconsistent with iOS and Android today, and no single declaration is correct for all three.I did not widen the type to
string | number, because that would force every consumer to narrow and would enshrine what looks like a platform bug. Windows is the lone outlier among the ten emission sites, so aligning it to strings seems like the better fix. That is a native change and a separate decision, so I have left it out of this PR — flagging it so it is on the record. (Windows also never emitsReactNativeBlobUtilProgress-uploadat all, souploadProgressdoes not fire there.)On
lstat(#257)You said "there are the same issues e.g. with lstat". I looked, and it is not the same shape of problem — which is why I have not bundled it in here:
sizeis declarednumber(index.d.ts:829) but emitted as a string on Android (ReactNativeBlobUtilFS.java:790,:800) and iOS (ReactNativeBlobUtilFS.mm:803builds anNSString), yet as a number on Windows (ReactNativeBlobUtil.cpp:1029,:1070useproperties.Size()).lastModifiedis declarednumberand is inconsistent within a single Android function:ReactNativeBlobUtilFS.java:791usesputInton the asset branch while:802usesputStringon the file branch. iOS returns anNSNumber(ReactNativeBlobUtilFS.mm:811); Windows returns a number.So
progressis a pure declaration bug with one unambiguous correct answer, whereaslstatneeds a native fix and a decision from you about which type wins before the declaration can be made truthful. Declaringsize: stringwould just move the lie to Windows. I would rather do that properly in a follow-up once you have said which direction you want; happy to pick it up.Verification
There is no unit test suite in this repo (
npm testexits 1, andtests/e2eneeds a device, Metro and a full build), and this change has no runtime component to exercise — so I verified it at the type level, which is where the bug actually lives.I compiled a throwaway fixture against
index.d.tswithtsc --noEmit --strict. It calls.padStart()on the callback params — legal at runtime, since they are strings. The fixture is not part of this PR; it was deleted before pushing.Before (this commit reverted):
Lines 8/13/18 are
progress(config, cb),uploadProgress(cb)anduploadProgress(config, cb). Theprogress(cb)control at line 22 was already correct and reported nothing.After (this commit applied): compiles clean,
exit=0.To make sure that was not just "everything compiles now", a second fixture asserts things that must remain errors, each guarded by
@ts-expect-error(which itself errors if the error it guards disappears). All four are still rejected after the change:TS2362/TS2363 is the consumer-visible break this PR introduces, pinned deliberately so it is not silent.
Breaking change
TypeScript and Flow users doing arithmetic directly on the params will now get a compile error. The fix is a one-liner at the call site:
No runtime behaviour changes, so untyped JavaScript consumers are unaffected.