Add tooling to verify dependency chain #3033

schmidt-sebastian · 2020-05-07T04:36:48Z

This adds tooling to verify top-level dependencies of tree-shakeable APIs. It is hooked up a a very basic version of a tree-shakeable API.

The goal of this API is to let us verify the dependency chain. One example where I used this is here: #3024

I suspect this might be helpful for other APIs and as such I added it to /scripts.

schmidt-sebastian · 2020-05-07T04:38:10Z

packages/firestore/exp/test/deps/dependencies.json

@@ -0,0 +1,15 @@
+{


Note: This file is generated by the Node CLI tool and then used in the unit test as the source of truth. It is checked in and as such provides a nice diff as we make changes.

"sizeInBytes" is for reference only.

avolkovi · 2020-05-07T21:33:34Z

packages/firestore/exp/test/deps/dependencies.json

+        "dependencies": [
+            "bar"
+        ],
+        "sizeInBytes": 39


this is per function? seems like a pain to always update

It is auto-updated via the script, but the script is not run automatically. I'll send this to @Feiyang1 and we can ponder how we can automate this step.

avolkovi

Looks cool, excited to see how it works in practice. Should we consider adding a pre submit hook that calls this to regenerate the .json?

Feiyang1

Cool stuff. We should definitely automate it and generate a nicely formatted report that we can review for every PR.

For some reason, I kept getting Error: ENOENT: no such file or directory, open '/Users/feiyangc/Projects/firebase-js-sdk/packages/firestore/dist/lib/src/util/error.d.ts' and couldn't compile the project and test the scripts. Any clue?

Feiyang1 · 2020-05-12T17:59:43Z

packages/firestore/exp/test/deps/verify_dependencies.test.ts

+        dependencies.sort();
+        expect(extractedDependencies).to.have.members(dependencies);


dependencies.sort(); is not necessary because order doesn't matter in .members().

Feiyang1 · 2020-05-12T19:02:21Z

scripts/exp/extract-deps.ts

+  return JSON.stringify(result, null, 4);
+}
+
+const publicApi = extractFunctionsAndClasses(path.resolve(argv.types));


It doesn't handle reexports, e.g. export * from 'src/impl'. We can generate a rollup d.ts file to sidestep the problem, or we need to traverse the AST to get all exports.

Isn't that more like the behavior we want though? We bundle all of our own sources into a single script. If one of our dependencies changes, we wouldn't want our tests to break.

Or are you planning to no longer ship a single bundled source file? That might break Firestore and other SDKs as we have a ton of transitive dependencies. Bundling works around that.

You are extracting APIs from the d.ts file, not the source file, right? Currently we don't bundle d.ts files into a single file, so there can be reexports from other d.ts files.

As we plan to have multiple entry points in firestore next, it will be easier to not have a single bundled source file so it's easier to share code between different entry points. Why would transitive dependencies be a problem though?

The public APIs are extracted from a handcrafted index.d.ts. Are we moving away from hand-crafted types? Note that this is not meant to work for all of Firebase, but only for each specific SDK.

FWIW, as more SDKs adopt this, we probably have to greatly expand the feature set of this tooling. I wonder if it makes sense to wait until there is a need, which can then happen together with CI integration?

I leave that decision to teams. But generally we don't need to handcraft d.ts anymore in the modular world, because we control what gets exported in the source code, and we can generate d.ts files with only public APIs from it automatically.

I generate index.d.ts from source code and use it for @firebase/app-exp. It allows me to keep documentation in the source code as well. I expect most of our SDKs to use automatically generated d.ts files in the modular world.

Sure. that sounds reasonable to me.

It would be interesting to see if we can hide SDK-public internals from the public typings without splitting up all Firestore classes into abstract Public-API-only base classes and implementation classes. I will keep an eye on this as we rewrite the client.

Feiyang1 · 2020-05-12T19:06:19Z

scripts/exp/extract-deps.helpers.ts

+  ts.forEachChild(sourceFile, node => {
+    if (ts.isFunctionDeclaration(node)) {
+      exports.push(node.name!.text);
+    } else if (ts.isClassDeclaration(node)) {
+      exports.push(node.name!.text);
+    }


It'd be interesting to see variable declarations too, e.g. the error message map.
It'd be nice to keep the types of the declarations and show them in the report, but it can be done in a separate PR.

Should be pretty easy to do as part of this PR. I will report back in my conversation with myself.

…js-sdk into mrschmidt/extractdeps

schmidt-sebastian

I don't know why you suddenly got the build failure. It seems unrelated to this PR. I was able to repro using a clean checkout though and fixed it. I hope there is no adverse size impact, but I am waiting for GitHub Actions to come back to confirm.

schmidt-sebastian · 2020-05-12T19:55:44Z

packages/firestore/exp/test/deps/verify_dependencies.test.ts

+        dependencies.sort();
+        expect(extractedDependencies).to.have.members(dependencies);


schmidt-sebastian · 2020-05-12T19:58:47Z

scripts/exp/extract-deps.helpers.ts

+  ts.forEachChild(sourceFile, node => {
+    if (ts.isFunctionDeclaration(node)) {
+      exports.push(node.name!.text);
+    } else if (ts.isClassDeclaration(node)) {
+      exports.push(node.name!.text);
+    }


Should be pretty easy to do as part of this PR. I will report back in my conversation with myself.

schmidt-sebastian · 2020-05-12T20:10:37Z

scripts/exp/extract-deps.helpers.ts

+  ts.forEachChild(sourceFile, node => {
+    if (ts.isFunctionDeclaration(node)) {
+      exports.push(node.name!.text);
+    } else if (ts.isClassDeclaration(node)) {
+      exports.push(node.name!.text);
+    }


schmidt-sebastian · 2020-05-12T21:19:50Z

scripts/exp/extract-deps.ts

+  return JSON.stringify(result, null, 4);
+}
+
+const publicApi = extractFunctionsAndClasses(path.resolve(argv.types));


Isn't that more like the behavior we want though? We bundle all of our own sources into a single script. If one of our dependencies changes, we wouldn't want our tests to break.

Or are you planning to no longer ship a single bundled source file? That might break Firestore and other SDKs as we have a ton of transitive dependencies. Bundling works around that.

google-oss-bot · 2020-05-12T23:46:16Z

Binary Size Report

Affected SDKs

No changes between base commit (ab2e73d) and head commit (b8ab8d9).

Test Logs

Base (ab2e73d6): https://github.com/firebase/firebase-js-sdk/actions/runs/104983370
Head (b8ab8d9d): https://github.com/firebase/firebase-js-sdk/actions/runs/105044893

schmidt-sebastian · 2020-05-13T04:19:57Z

@Feiyang1 The quick and dirty fix for your build issue increased the size of the SDK. Instead, I updated extract-api.ts to perform a precompile step.

schmidt-sebastian · 2020-05-13T04:20:31Z

scripts/emulator-testing/emulators/emulator.ts

@@ -40,7 +40,7 @@ export abstract class Emulator {

  download(): Promise<void> {
    return new Promise<void>((resolve, reject) => {
-      tmp.dir((err: Error, dir: string) => {
+      tmp.dir((err: Error|null, dir: string) => {


This was needed to fix the CI.

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners May 7, 2020 04:36

schmidt-sebastian commented May 7, 2020

View reviewed changes

Add tooling to verify dependency chain

f21776b

schmidt-sebastian force-pushed the mrschmidt/extractdeps branch from 9891d6e to f21776b Compare May 7, 2020 04:47

Update extract-deps.sh

ea41857

schmidt-sebastian added the api: firestore label May 7, 2020

schmidt-sebastian requested a review from avolkovi May 7, 2020 04:48

schmidt-sebastian assigned avolkovi May 7, 2020

avolkovi assigned sam-gc May 7, 2020

avolkovi reviewed May 7, 2020

View reviewed changes

avolkovi approved these changes May 7, 2020

View reviewed changes

avolkovi removed their assignment May 7, 2020

schmidt-sebastian assigned Feiyang1 May 7, 2020

Feiyang1 reviewed May 12, 2020

View reviewed changes

schmidt-sebastian added 4 commits May 12, 2020 14:45

Review

7df1038

[AUTOMATED]: Prettier Code Styling

035091c

Merge branch 'mrschmidt/extractdeps' of github.com:firebase/firebase-…

2124354

…js-sdk into mrschmidt/extractdeps

Fix build

73ea558

schmidt-sebastian commented May 12, 2020

View reviewed changes

schmidt-sebastian added 3 commits May 12, 2020 18:36

Use generated .d.ts files

9544284

[AUTOMATED]: Prettier Code Styling

040f141

[AUTOMATED]: License Headers

ea361a4

schmidt-sebastian force-pushed the mrschmidt/extractdeps branch from 0f3f537 to 8203d4d Compare May 13, 2020 01:37

Better comment

4701cde

schmidt-sebastian force-pushed the mrschmidt/extractdeps branch from 8203d4d to 4701cde Compare May 13, 2020 01:38

Fix build

a0e8c26

schmidt-sebastian commented May 13, 2020

View reviewed changes

Feiyang1 approved these changes May 14, 2020

View reviewed changes

Merge branch 'master' into mrschmidt/extractdeps

f8995f0

schmidt-sebastian merged commit 94ee69a into master May 14, 2020

firebase locked and limited conversation to collaborators Jun 14, 2020

schmidt-sebastian deleted the mrschmidt/extractdeps branch July 9, 2020 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tooling to verify dependency chain #3033

Add tooling to verify dependency chain #3033

schmidt-sebastian commented May 7, 2020

schmidt-sebastian May 7, 2020 •

edited

Loading

avolkovi May 7, 2020

schmidt-sebastian May 7, 2020

avolkovi left a comment

Feiyang1 left a comment

Feiyang1 May 12, 2020

schmidt-sebastian May 12, 2020

Feiyang1 May 12, 2020

schmidt-sebastian May 12, 2020

Feiyang1 May 13, 2020

schmidt-sebastian May 13, 2020

Feiyang1 May 14, 2020 •

edited

Loading

schmidt-sebastian May 14, 2020

Feiyang1 May 12, 2020

schmidt-sebastian May 12, 2020

schmidt-sebastian May 12, 2020

schmidt-sebastian left a comment •

edited

Loading

schmidt-sebastian May 12, 2020

schmidt-sebastian May 12, 2020

schmidt-sebastian May 12, 2020

schmidt-sebastian May 12, 2020

google-oss-bot commented May 12, 2020 •

edited

Loading

schmidt-sebastian commented May 13, 2020

schmidt-sebastian May 13, 2020

		dependencies.sort();
		expect(extractedDependencies).to.have.members(dependencies);

Add tooling to verify dependency chain #3033

Add tooling to verify dependency chain #3033

Conversation

schmidt-sebastian commented May 7, 2020

schmidt-sebastian May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avolkovi left a comment

Choose a reason for hiding this comment

Feiyang1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feiyang1 May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented May 12, 2020 • edited Loading

Binary Size Report

Affected SDKs

Test Logs

schmidt-sebastian commented May 13, 2020

Choose a reason for hiding this comment

schmidt-sebastian May 7, 2020 •

edited

Loading

Feiyang1 May 14, 2020 •

edited

Loading

schmidt-sebastian left a comment •

edited

Loading

google-oss-bot commented May 12, 2020 •

edited

Loading