Add an interface Database #3511
Conversation
🦋 Changeset is good to goLatest commit: 12f8c00 We got this. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Binary Size ReportAffected SDKs
Test Logs |
|
|
||
| export class FirebaseDatabase implements Database { |
There was a problem hiding this comment.
Since this is a .d.ts file, isn't "class" already considered an interface? Could we just drop private constructor?
There was a problem hiding this comment.
IIUC, we use it to type the actual class that we export and we don't want people to instantiate it, so private constructor is important. Please correct me if i'm wrong.
There was a problem hiding this comment.
You can use d.ts file to annotate a JS library. It tells Typescript that the JS library exports a class that people can import and use. People can actually do this and Typescript allows it:
import {FirebaseDatabase} from '@firebase/database-types';
new FirebaseDatabase(); // if not having the private constructor
For this reason, I consider it as a bad practice to use class instead of interface in our types packages, if you are just going for the instance type.
There was a problem hiding this comment.
If I understand this correctly, it seems like the following two are equivalent:
export class Foo {
private constructor();
foo: string;
}
export interface Foo {
foo: string;
}
If so, can we drop the existing class declaration? If not, this LGTM.
There was a problem hiding this comment.
They are the same for representing the instance type, but we need the constructor type which requires a class type. See https://github.com/firebase/firebase-js-sdk/blob/master/packages/database/index.ts#L97
I believe it is to allow people to do instanceof, but disallow newing an object, so we need to keep the class type.
There was a problem hiding this comment.
Yeah, the instanceof check was the only thing that I could think of, but it seemed a bit unlikely. Better not to break anyone though.
Co-authored-by: Sebastian Schmidt <[email protected]>
The admin SDK wants to export the type of the instance of the
FirebaseDatabaseclass in their namespace, but encountered an issue where Typescript thinks they are exporting the actual class from@firebase/database-typesand compiles it to code that leads to runtime error:Adding an interface that's implemented by
FirebaseDatabaseto work around the issue.I tried to use
InstanceType<typeof FirebaseDatabase>to get the type of the instance without creating a new type, but it doesn't work because the constructor is private 🤷 .