Add API Extractor for Firestore + API Pruning #3790

schmidt-sebastian · 2020-09-16T01:46:52Z

This adds support for API extractor to Firestore Exp and Firestore Lite and a script to prune its output to match our previous manual API layer.

I manually verified that the new and the old type files are equivalent (which was quite painful, but very necessary).

There are some things we could still improve:

Remove unused imports
Go back to export instead of export declare
Whitespace between lines.

changeset-bot · 2020-09-16T01:46:54Z

⚠️ No Changeset found

Latest commit: 9f9cc75

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2020-09-16T02:01:04Z

Size Analysis Report

Affected Products

No changes between base commit (b6925be) and head commit (dc8d06a).

Test Logs

Base (b6925be1): https://github.com/firebase/firebase-js-sdk/actions/runs/283283618
Head (dc8d06a3): https://github.com/firebase/firebase-js-sdk/actions/runs/283336390

google-oss-bot · 2020-09-16T02:02:49Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`b6925be`)	Head (`dc8d06a`)	Diff
browser	249 kB	249 kB	+96 B (+0.0%)
esm2017	197 kB	197 kB	+96 B (+0.0%)
main	484 kB	484 kB	-9 B (-0.0%)
module	247 kB	247 kB	+96 B (+0.0%)
react-native	197 kB	197 kB	+96 B (+0.0%)

@firebase/firestore-exp

Type Base (b6925be) Head (dc8d06a) Diff

browser ? 189 kB ? (?)

main ? 477 kB ? (?)

module ? 189 kB ? (?)

react-native ? 189 kB ? (?)
@firebase/firestore-lite

Type Base (b6925be) Head (dc8d06a) Diff

browser ? 63.5 kB ? (?)

main ? 140 kB ? (?)

module ? 63.5 kB ? (?)

react-native ? 63.7 kB ? (?)

`@firebase/firestore/memory`

Type	Base (`b6925be`)	Head (`dc8d06a`)	Diff
browser	187 kB	187 kB	+96 B (+0.1%)
esm2017	147 kB	147 kB	+96 B (+0.1%)
main	357 kB	357 kB	-9 B (-0.0%)
module	185 kB	185 kB	+96 B (+0.1%)
react-native	147 kB	148 kB	+96 B (+0.1%)

firebase

Type Base (b6925be) Head (dc8d06a) Diff

firebase-firestore.js 287 kB 287 kB +94 B (+0.0%)

firebase-firestore.memory.js 227 kB 227 kB +94 B (+0.0%)

firebase.js 831 kB 831 kB +94 B (+0.0%)

Test Logs

Base (b6925be1): https://github.com/firebase/firebase-js-sdk/actions/runs/283283618
Head (dc8d06a3): https://github.com/firebase/firebase-js-sdk/actions/runs/283336390

Feiyang1 · 2020-09-16T17:38:55Z

packages/firestore/exp-types/index.d.ts

@@ -1,544 +1,557 @@
-/**


Should this file be not committed since it's generated now?

I prefer not deleting it yet as it makes the changes to this tool and the API in general much more visible. A next step is to use the new d.ts file as input to API Explorer. I think we can remove it once that is done.

packages/firestore/exp/src/api/database.ts

Feiyang1 · 2020-09-16T17:51:40Z

packages/firestore/rollup.config.exp.js

+      },
+      {
+        // Copy into generated source files to support API Extractor
+        src: 'src/protos/firestore_proto_api.d.ts',


You can change firestore_proto_api.d.ts to a ts file even though it only has types, so TS will automatically generate the d.ts file for you in the dist folder.

That is L5 level genius.

Feiyang1 · 2020-09-16T17:53:19Z

packages/firestore/scripts/prune-dts.ts

+  input: {
+    type: 'string',
+    demandOption: true,
+    desc: 'The location of the index.ts file'


s/index.ts/index.d.ts

Feiyang1 · 2020-09-16T17:54:26Z

packages/firestore/scripts/prune-dts.ts

+ * - Constructors are made private or protected if marked with
+ *   `@hideconstructor`/`@hideconstructor protected`.
+ *
+ * This function is meant to operate on DTS files generated by API explorer


s/explorer/extractor

Feiyang1 · 2020-09-16T18:40:46Z

packages/firestore/scripts/prune-dts.ts

+ * export class PublicFoo {}
+ * export function doFoo(foo: PublicFoo);
+ */
+function extractPublicName(


Why would we use a private type in a public API? This feels like an issue that should be fixed in the source code. In case of multiple public classes extending PrivateFoo, we use the first one which seems arbitrary and is probably wrong - considering PublicFoo1 and PublicFoo2, both of which extends PrivateFoo. doFoo should accept either of them, but after this change, it will only accept PublicFoo1.

This is definitely an issue in our source code, but one that saves us 1000+ lines of code. The Lite SDK and the Exp SDK use the same Collection/Document/Query types and APIs, even though the actual instance of FirebaseFirestore/FirebaseDataConverter is different. If I don't perform this hack, then two versions of FirebaseDataConverter exist and all APIs that seemingly depend on the Lite type leak types that are invisible.

Can you show me an example in a snippet? Don't lite and exp have separate d.ts files, why would there be 2 versions of FirebaseDataConverter? and what do you mean by leaking invisible types?

Feiyang1 · 2020-09-16T18:51:17Z

packages/firestore/scripts/prune-dts.ts

+          exportedTypes.push(
+            ts.updateExpressionWithTypeArguments(
+              type,
+              type.typeArguments,


What if the public class/interface takes different numbers of type arguments? Will it create broken typings for this example?

class A<T> { data: T; } class B extends A<string> { }

Feiyang1 · 2020-09-16T19:09:30Z

packages/firestore/scripts/prune-dts.ts

+            ts.updateExpressionWithTypeArguments(
+              type,
+              type.typeArguments,
+              ts.createIdentifier(publicName)


IIUC, this can potentially make the class/interface has more members than it really has. Considering this:

class A { data: string; } export class C extends A { additionalData: number; } export class B extends A { }

will become

export class C { data: string; additionalData: number; } export class B extends C { }

Now B has additionalData it didn't have before.

I think the safest strategy is always copy the members from the private symbols instead of replacing it with a public one that extends it.

This becomes:

export declare class B2 { data?: string; } export declare class C2 extends B2 { additionalData?: number; }

That seems correct to me.

I'm confused. Is B2 B and C2 C from my example? Or are you giving a different example?

Feiyang1 · 2020-09-16T19:15:44Z

Nit: I saw some usage of == and != which should be replaced by === and !== instead.

schmidt-sebastian

Here it is again.

Sorry, the commit history is a bit messy, but I wanted to keep it in case I need to go back to an earlier revision.

I addressed your main two points as follows:

If a private API has different generic arguments from a public API, I will no longer simply substitute their names. Instead, I merge the declarations of the private API into the public type.
To merge the declarations, I now use TypeScript's codefix package. At first, I tried to just port the code that we need, but it quickly got out of hand. So for now, this script relies on the same private API that VS Code uses to auto-fill missing imports. If this private API ever breaks, we can move the required parts into our repository (a hacky attempt is here: ffc5ba1#diff-6db0153ee9f6f29fa6fc10782448b200R204, but it still has some issues)

packages/firestore/exp/src/api/database.ts

schmidt-sebastian · 2020-09-16T21:22:04Z

packages/firestore/exp-types/index.d.ts

@@ -1,544 +1,557 @@
-/**


I prefer not deleting it yet as it makes the changes to this tool and the API in general much more visible. A next step is to use the new d.ts file as input to API Explorer. I think we can remove it once that is done.

schmidt-sebastian · 2020-09-16T21:23:25Z

packages/firestore/rollup.config.exp.js

+      },
+      {
+        // Copy into generated source files to support API Extractor
+        src: 'src/protos/firestore_proto_api.d.ts',


That is L5 level genius.

schmidt-sebastian · 2020-09-16T21:29:53Z

packages/firestore/scripts/prune-dts.ts

+ * export class PublicFoo {}
+ * export function doFoo(foo: PublicFoo);
+ */
+function extractPublicName(


This is definitely an issue in our source code, but one that saves us 1000+ lines of code. The Lite SDK and the Exp SDK use the same Collection/Document/Query types and APIs, even though the actual instance of FirebaseFirestore/FirebaseDataConverter is different. If I don't perform this hack, then two versions of FirebaseDataConverter exist and all APIs that seemingly depend on the Lite type leak types that are invisible.

schmidt-sebastian · 2020-09-16T21:39:24Z

packages/firestore/scripts/prune-dts.ts

+            ts.updateExpressionWithTypeArguments(
+              type,
+              type.typeArguments,
+              ts.createIdentifier(publicName)


This becomes:

export declare class B2 { data?: string; } export declare class C2 extends B2 { additionalData?: number; }

That seems correct to me.

Feiyang1 · 2020-09-30T05:03:59Z

packages/firestore/scripts/prune-dts.ts

+        if (
+          publicSymbol &&
+          publicSymbol.name !== currentName &&
+          verifyMatchingTypeArguments(


I'm not sure it's the sufficient condition for a safe replacement. Considering:

declare class A<T> { aData: T; } export declare class B<T> extends A<string> { bData: T; } export declare class C<T> extends A<T> { }

Will it become?

export declare class B<T> { aData: string; bData: T; } export declare class C<T> extends B<T> { // it is incorrect, because the type of `aData` becomes `string` which was `T` in the original definition. }

Feiyang1 · 2020-09-30T05:08:56Z

packages/firestore/scripts/prune-dts.ts

+  // can explore adding a slimmed down version of this package to our repository
+  // if this dependency should ever break.
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  (ts as any).codefix.createMissingMemberNodes(


Does it handle generics with different names? For example:

declare class A<T> { aData: T; } export declare class B<K> extends A<K> { bData:K; }

Does it become:

export declare class B<K> { aData: K; // instead of aData: T bData: K; }

Feiyang1 · 2020-09-30T05:19:03Z

packages/firestore/scripts/prune-dts.ts

+ * export class PublicFoo {}
+ * export function doFoo(foo: PublicFoo);
+ */
+function extractPublicName(


Can you show me an example in a snippet? Don't lite and exp have separate d.ts files, why would there be 2 versions of FirebaseDataConverter? and what do you mean by leaking invisible types?

schmidt-sebastian · 2020-10-02T00:05:08Z

@Feiyang1 - Can you take another look at this?

Feiyang1 · 2020-10-02T16:26:06Z

@schmidt-sebastian I don't see any new changes since my last review. Did you mean to push something, but didn't?

Add Api Extractor + Pruning

fa67d00

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners September 16, 2020 01:46

schmidt-sebastian assigned Feiyang1 Sep 16, 2020

schmidt-sebastian added the api: firestore label Sep 16, 2020

schmidt-sebastian changed the title ~~Add API Extractor for Firestor + API Pruning~~ Add API Extractor for Firestore + API Pruning Sep 16, 2020

Lint

f6c582d

Feiyang1 reviewed Sep 16, 2020

View reviewed changes

schmidt-sebastian added 14 commits September 16, 2020 14:30

Easy fixes

e72e778

More fixes

85c8738

WIP

a0bc324

Try to infer type

8e91691

WIP

10d1d7e

Almost

180751f

WIP

c014c87

Cleanup

ffc5ba1

Before calling into private API

95eb38c

Before cleanup

e9a653e

Cleanup

4f590b2

Fix prettierignore

4280552

Re-run

0cc4f95

Lint

8ebce61

schmidt-sebastian commented Sep 28, 2020

View reviewed changes

schmidt-sebastian added 2 commits September 28, 2020 14:43

Merge master

d12d5f0

Re-gen docs

203da28

Feiyang1 reviewed Sep 30, 2020

View reviewed changes

Merge

ed35559

schmidt-sebastian requested review from avolkovi and sam-gc as code owners October 2, 2020 00:05

Feiyang1 mentioned this pull request Oct 2, 2020

Reference docs for Auth next #3883

Merged

Feiyang1 assigned schmidt-sebastian and unassigned Feiyang1 Oct 2, 2020

WIP

50dd379

This was referenced Nov 11, 2020

Accept Compat types in firestore-exp API #4056

Merged

Compat class for Query and CollectionReference #4065

Merged

schmidt-sebastian added 4 commits November 18, 2020 09:31

Merge

508c86a

Cleanup

587360a

Add to scripts folder

46a0b67

Prettier

9f9cc75

schmidt-sebastian mentioned this pull request Nov 19, 2020

Script to Prune .d.ts files (with tests) #4091

Merged

schmidt-sebastian closed this Nov 25, 2020

schmidt-sebastian deleted the mrschmidt/pruneapi branch November 25, 2020 00:02

firebase locked and limited conversation to collaborators Dec 25, 2020

Add API Extractor for Firestore + API Pruning #3790

Add API Extractor for Firestore + API Pruning #3790

Uh oh!

Conversation

schmidt-sebastian commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

google-oss-bot commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Analysis Report

Affected Products

Test Logs

Uh oh!

google-oss-bot commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore-exp

@firebase/firestore-lite

@firebase/firestore/memory

firebase

Test Logs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Feiyang1 commented Sep 16, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Oct 2, 2020

Uh oh!

Feiyang1 commented Oct 2, 2020

Uh oh!

Uh oh!

schmidt-sebastian commented Sep 16, 2020 •

edited

Loading

changeset-bot bot commented Sep 16, 2020 •

edited

Loading

google-oss-bot commented Sep 16, 2020 •

edited

Loading

google-oss-bot commented Sep 16, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/memory`

`firebase`