Skip to content

fix(types): declare progress/uploadProgress callback params as string - #484

Open
giaBaoJS wants to merge 1 commit into
RonRadtke:masterfrom
giaBaoJS:fix/progress-callback-types
Open

fix(types): declare progress/uploadProgress callback params as string#484
giaBaoJS wants to merge 1 commit into
RonRadtke:masterfrom
giaBaoJS:fix/progress-callback-types

Conversation

@giaBaoJS

Copy link
Copy Markdown

Fixes #410.

The progress() overloads contradict each other: progress(callback) declares (received: string, total: string) while progress(config, callback) declares (received: number, total: number) — even though both are fed by the same ReactNativeBlobUtilProgress event. The declaration that says number is 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/total on iOS and Android stringifies:

Platform File:line Event Emits
Android ReactNativeBlobUtilReq.java:225-226 EVENT_PROGRESS putString(String.valueOf(...))
Android Response/ReactNativeBlobUtilDefaultResp.java:73-74 EVENT_PROGRESS putString(String.valueOf(...))
Android Response/ReactNativeBlobUtilFileResp.java:170-171 EVENT_PROGRESS putString(String.valueOf(...))
Android ReactNativeBlobUtilBody.java:441-442 EVENT_UPLOAD_PROGRESS putString(String.valueOf(...))
iOS ReactNativeBlobUtilRequest.mm:428-429, 436-437 EVENT_PROGRESS [NSString stringWithFormat:]
iOS ReactNativeBlobUtilRequest.mm:478-479 EVENT_PROGRESS [NSString stringWithFormat:]
iOS ReactNativeBlobUtilRequest.mm:655-656 EVENT_PROGRESS [NSString stringWithFormat:]
iOS ReactNativeBlobUtilRequest.mm:563-564 EVENT_PROGRESS_UPLOAD [NSString stringWithFormat:]

fetch.js:195 and fetch.js:202 pass these straight through to the user callback, so nothing converts them on the way.

What changed

  • index.d.tsprogress(config, callback) and both uploadProgress overloads now declare string.
  • index.js.flowProgressCallback and UploadProgressCallback likewise. Without this the two type systems shipped in the package would disagree with each other.
  • README.md — the four progress examples used received / total, which is fine at runtime but stops compiling under the corrected types. They now convert explicitly, which doubles as the migration hint.

Why uploadProgress is included

uploadProgress declares number in both overloads and is wrong in both — it reads the same written/total fields, emitted as strings from ReactNativeBlobUtilBody.java:441-442 and ReactNativeBlobUtilRequest.mm:563-564. Fixing only progress() 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:

  • (a) change the types to 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, since received / total coerces.
  • (b) change native to emit numbers — no compile-time break, but a runtime break for everyone currently doing 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 stringnumber so that Math.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-1807 emits written as int64_t and total as a UInt64 or null — 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 emits ReactNativeBlobUtilProgress-upload at all, so uploadProgress does 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:

  • size is declared number (index.d.ts:829) but emitted as a string on Android (ReactNativeBlobUtilFS.java:790, :800) and iOS (ReactNativeBlobUtilFS.mm:803 builds an NSString), yet as a number on Windows (ReactNativeBlobUtil.cpp:1029, :1070 use properties.Size()).
  • lastModified is declared number and is inconsistent within a single Android function: ReactNativeBlobUtilFS.java:791 uses putInt on the asset branch while :802 uses putString on the file branch. iOS returns an NSNumber (ReactNativeBlobUtilFS.mm:811); Windows returns a number.

So progress is a pure declaration bug with one unambiguous correct answer, whereas lstat needs a native fix and a decision from you about which type wins before the declaration can be made truthful. Declaring size: string would 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 test exits 1, and tests/e2e needs 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.ts with tsc --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):

tsprobe-flip.ts(8,26): error TS2339: Property 'padStart' does not exist on type 'number'.
tsprobe-flip.ts(8,51): error TS2339: Property 'padStart' does not exist on type 'number'.
tsprobe-flip.ts(13,22): error TS2339: Property 'padStart' does not exist on type 'number'.
tsprobe-flip.ts(13,47): error TS2339: Property 'padStart' does not exist on type 'number'.
tsprobe-flip.ts(18,22): error TS2339: Property 'padStart' does not exist on type 'number'.
tsprobe-flip.ts(18,47): error TS2339: Property 'padStart' does not exist on type 'number'.
exit=2

Lines 8/13/18 are progress(config, cb), uploadProgress(cb) and uploadProgress(config, cb). The progress(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:

error TS2339: Property 'notARealMethod' does not exist on type 'string'.
error TS2362: The left-hand side of an arithmetic operation must be of type 'any', 'number', 'bigint' or an enum type.
error TS2363: The right-hand side of an arithmetic operation must be of type 'any', 'number', 'bigint' or an enum type.
error TS2322: Type 'string' is not assignable to type 'number'.   // config { count: 'ten' }
error TS2345: Argument of type '(received: string, total: string, chunk: string) => void' is not assignable to
              parameter of type '(received: string, total: string) => void'.

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:

.progress({count: 10}, (received, total) => {
    const pct = Number(received) / Number(total);
})

No runtime behaviour changes, so untyped JavaScript consumers are unaffected.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error type of params in callback of StatefulPromise<T>.progress()

1 participant