firebase / firebase/firebase-js-sdk

Lacking build-time error when doc() is used with collectionRef and `undefined` path

Open
#7,826 2 comments 6 reactions 1 assignee Claimed by @cherylEnkidu View on GitHub
api: firestore feature request
Dominant language
TypeScript
Stars
5.1k
Forks
1k
Avg merge
2d 21h
Merged PRs (30d)
37

Description

### Operating System

MacOS Sonoma 14.1.1

### Browser Version

n/a

### Firebase SDK Version

10.6.0

### Firebase SDK Product:

Firestore

### Describe your project's tooling

n/a

### Describe the problem

When using the Firestore doc function with TypeScript, there's an issue with the type overloads that allows passing `undefined` as a path parameter when the first argument is a `CollectionReference`. This should not be allowed according to the Firestore SDK's design.

Currently, `doc(collectionRef, undefined)` or `doc(collectionRef, someVariableThatMayBeUndefined)` is allowed, but a __runtime error is thrown__. Ideally, `undefined` should be caught at compile time rather than resulting in a runtime error.

---

FWIW, I did some explorations and right now the overloads for the `doc()` functions are:

```ts
declare function doc(
firestore: Firestore,
path: string,
...pathSegments: string[]
): DocumentReference;

declare function doc(
reference: CollectionReference,
path?: string,
...pathSegments: string[]
): DocumentReference;

declare function doc(
reference: DocumentReference,
path: string,
...pathSegments: string[]
): DocumentReference;
```

The problematic one is the second. The optional `path?: string` makes it possible to pass an `undefined` value, resulting in a unexpected runtime error.

If we were to __replace__ it with two other overloads, where the first is for `doc(collectionRef)` ([which I think makes sense considering how the `autoId` check works](https://github.com/firebase/firebase-js-sdk/blob/70e4cf6a6544c4ccfa609c3f2c320980e7122101/packages/firestore/src/lite-api/reference.ts#L573)) and the second for `doc(collectionRef, path, ...optionalSegments)`

```ts
declare function doc(
reference: CollectionReference,
): DocumentReference;

declare function doc(
reference: CollectionReference,
path: string,
...pathSegments: string[]
): DocumentReference;
```

The issue would be caught at build time instead of at runtime. If I'm not missing anything, I'm more than happy to open a PR for this.

---

On a light tangential note, is there a reason why the overload for `doc(firestore, path)` doesn't allow generics to be provided and force `DocumentReference`?

Could it be like the following, defaulting the generics to `DocumentData` as it is right now?

```ts
declare function doc(
firestore: Firestore,
path: string,
...pathSegments: string[]
): DocumentReference;
```

The same goes for `collection(firestore, path)`

### Steps and code to reproduce issue

I've built a [minimal repro in TS playground](https://www.typescriptlang.org/play?ssl=53&ssc=16&pln=53&pc=25#code/CYUwxgNghgTiAEYD2A7AzgF3mgYgSzkyTgC54BXFPAR3ITQE8BbAIyQgChRJYFl0saACJIw5JiBQYASiABmIOCjAgylGnWzM2nbtDiJUmbAGF2EcBjypZCpSrVVa9bew4cMDAA4J8hDMQIALzwnj5Icth+IERwHt4IImISUraKkioAPAAq8CFQKAwANPAAqnnwBQwAfBVhIBHYSeKSMvLpyiDxPvBmEBZgVjbt9iA5FVUl5fmFtSH1jWh9A0MoaaPdiaItUkJQGFAVssgwwJmYMHgoAOYlVdXuHAD0T4YwcIMleNcogaEAFng0FxwPoEHJKINrCh4MBRAAKDjweByAgxAKkeDRWIgIpI+Befb-MgXK63fEAOiphIw-wAyiBrikMGgSRhLjcANoAXQ4AEoyM1mesMmMha09gcSuLdvsoNUANzuF7wbKAtDwIEAvjkd6teBIABuiggSCgwBKyCYzIBWoKwAoyiQ1v1tIQKBAAA8sBgAO5IEE8AwQ5SrWGiTIAQS8XgAskhQBBsglpSx44nkz0vRhJMANTKMJL5YjkXA7KKyMtLNCRZ0ozH0yAkyn4EI0wmm5mQNU8ciaf8APxsjnk5FUin9hlM1qs7Dssk8-mC7bCkai+txjvNnypxvb7tK56vSNyHMwQwuqRk7UG40wU3mypsY0lSHOm20rVGk1m4BH2GgrwKKQmGcJgBue5druW5dvA2a5vmK4SnK1SIiqpZrp0lbmNWwzlnW0abhmLZtpBCQ9v+AqtkhqSYVkhFkTurbtsRPiKsqx6nooF7Mtebq3j+j5QM+uKOlaH7qgJ96-v+ehASGUKoOG4EMTBJEsZ2CRwd6CHUckyEHKh-4YfhDi9DhilrHRYyqaxomkWpbF4uhBJEsOZLOa8Y7UkSU7MrOpJcryKpUQWtb0Q2jn2Rp+7sf+JwfBgXw-H8n7AnJwYgdCykQVF0F2dpOYoHmek7IWKElvAZYdGZYXWbldn5ZpTn4v27k3L28DjpOjL+e11yLqFNFtKZYrDUW0rjShh78MYqL+IEZDYhiwTwAA3gAvpUGrLYEHCzVgyD9LhVlyNhx2WeFq2bdt5kXasV37UYWBgWVtjLvptGjRUN1QIhn0jTVXQHfATBQAwLAgHSzogAAksA-XwAAPo6oCoh6DohJQaNXCAf6yQifLwL6eC0iiaI4lwCLzeigQlAA5GgMPw-TRMqjTOLwAA1K5tIlEgADWZCAKDkVNgPCHMrQzTMSCz0u9a0ACM9Py9OUgAEys-A7MUyt3O8-8+toArUhoPzQvwKLYsS7rdOo-IuPAGzryS38PO-AbJRoP8SDkBADqKDAxCW9brtwM75MLQY7tIJ72A+37AfvMQIsccp8JEyTZNHSs2VltbOcnbYEeF5Z+se-25up2B8Klw98jS8zwBayqdfZTzlcGhbos123eGq8yysD60mslxZYYd0SVch73481g38CM03KuL8basYEPq8mxgo-a68fcwpPZM82v-nT1bs-3fPcivsVDsY2PV9KTzXpeBAeBgKT9vo3jcfe77-s4LJxgGQQAMuQFznv3UG4NIbQ1lk7Pehgn6H2-o7eAhpYB4GEhYP+CdAGBxTvAcBBNxaZ1JobV6Np8410oa0Wwjd4Et1eGBKq8h9ad0FtXBEtCvoMLhs3YeUhlYRxYWWdhRIjbbzNl3VO1seEjREaIVhkQY64IAUnIOICZ7cOGvQ6BEMoZN0UWAZR+swYGNQRjNRicgGaNkUAA). It's a over simplification of the codebase, but demonstrates the issue and a possible fix.

---

Here's another minimal repro with the current SDK:

```ts
import { initializeApp } from 'firebase/app'
import { initializeFirestore, collection, doc } from 'firebase/firestore'

const testApp = initializeApp({
apiKey: 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
projectId: 'some-id',
})

const testFirestore = initializeFirestore(testApp)

const collectionRef = collection(testFirestore, 'users')
const userId = 'some-user-id'
const maybeUserId: string | undefined = undefined

// userId is string, that's fine
const userRef1 = doc(collectionRef, userId)

// maybeUserId can be undefined, should not be allowed at build time, but is
const userRef2 = doc(collectionRef, maybeUserId)

// explicit undefined, should not be allowed at build time, but is
const userRef3 = doc(collectionRef, undefined)
```

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.