firebase / firebase/firebase-js-sdk
Lacking build-time error when doc() is used with collectionRef and `undefined` path
- 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
Assessment
This issue has not been assessed yet.