firebase / firebase/firebase-js-sdk
RTDB should use JSON types instead of any
- Dominant language
- TypeScript
- Stars
- 5.1k
- Forks
- 1k
- Avg merge
- 2d 21h
- Merged PRs (30d)
- 37
Description
### Describe your environment
* Operating System version: N/A (type bug)
* Browser version: N/A (type bug)
* Firebase SDK version: 9.0.2
* Firebase Product: database
### Describe the problem
The Firebase Realtime Database uses `any` to represent a valid JSON value, but this allows customers to accidentally use disallowed values, such as [this SO question](https://stackoverflow.com/questions/69255685/firebase-transaction-completes-with-null-value-and-no-errors/69261915#69261915)
#### Relevant Code:
```
// Notice that this handler uses `async`, so its return type is Promise, not
// number. We don't handle this case.
const counterTxn = await runTransaction(firebaseRef, async node => {
if (!node) { return 1 };
return +node + 1;
});
```
This code compiles cleanly because we asked for a handler that takes an `any` and returns an `any`, but the customer is returning a `Promise` thanks to the `async` keyword and we definitely don't handle Promises in `transaction`.
### Suggestion
We should be much more explicit about our type info. For example, if we add the following type:
```
type JsonValue = string | number | boolean | null | Array | Record;
```
We can now be much more explicit in our SDK and turn some runtime errors into compile-time errors
```
export interface DataSnapshot {
exportVal(): JsonValue;
toJSON(): JsonValue;
val(): JsonValue;
}
export interface OnDisconnect {
set(value: JsonValue, onComplete?: (a: Error | null) => any): Promise
setWithPriority(
value: JsonValue,
priority: number | string | null,
onCompelte?: (a: Error | null) => any
): Promise
update(values: Record, onComplete?: (a: Error | null) => any): Promise;
}
export interface Reference extends Query {
push(value?: JsonValue, onComplete?: (a: Error | null) => any): ThenableReference;
set(value?: JsonValue, onComplete?: (a: Error | null) => any): Promise;
setWithPriority(
newVal: JsonValue,
newPriority: string | number | null,
onComplete?: (a: Error | null) => any
): Promise
transaction(
transactionUpdate: (a: JsonValue) => JsonValue | undefined,
onComplete?: (a: Error | null, b: boolean, c: DataSnapshot | null) => any,
applyLocally?: boolean
): Promise;
update(values: Record, onComplete?: (a: Error | null) => any): Promise;
}
```
If we used these more descriptive typings, then the Stack Overflow asker would instead have had an error:
```
Argument of type '() => Promise' is not assignable to parameter of type '(a: JsonValue) => JsonValue | undefined'
```
in the above sample
Contributor guide
Assessment
This issue has not been assessed yet.