firebase / firebase/firebase-js-sdk

RTDB should use JSON types instead of any

Open
#5,526 2 comments 0 reactions 1 assignee Claimed by @maneesht View on GitHub
api: database needs-attention question v9
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.