Remove unused code #2817

schmidt-sebastian · 2020-03-28T00:15:01Z

This PR removes unused code from the SDK (as shown by running the integration tests with coverage). It's only a small improvement, but it should not be harmful.

google-oss-bot · 2020-03-28T00:22:51Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`aab78f9`)	Head (`2767824`)	Diff
@firebase/firestore/memory	browser	224475.00	223853.00	-622.00 (-0.28%)
	module	221655.00	221033.00	-622.00 (-0.28%)
	esm2017	170304.00	169771.00	-533.00 (-0.31%)
	main	397404.00	396478.00	-926.00 (-0.23%)
@firebase/firestore	browser	271055.00	270233.00	-822.00 (-0.30%)
	module	267834.00	267012.00	-822.00 (-0.31%)
	esm2017	216141.00	215482.00	-659.00 (-0.30%)
	main	488418.00	487037.00	-1381.00 (-0.28%)
firebase	firebase.js	846872.00	846061.00	-811.00 (-0.10%)
	firebase-firestore.memory.js	266750.00	266140.00	-610.00 (-0.23%)
	firebase-firestore.js	311995.00	311184.00	-811.00 (-0.26%)

Metric Unit: byte

Test Logs

dconeybe · 2020-03-30T13:35:07Z

packages/firestore/src/util/async_queue.ts

-          // Escape the promise chain and throw the error globally so that
-          // e.g. any global crash reporting library detects and reports it.
-          // (but not for simulated errors in our tests since this breaks mocha)
-          if (message.indexOf('Firestore Test Simulated Error') < 0) {


This is still used, isn't it?

firebase-js-sdk/packages/firestore/test/unit/util/async_queue.test.ts

Line 75 in bd85150

const expected = new Error('Firestore Test Simulated Error');

You mentioned in the PR description that you ran the integration tests... did you run the unit tests also? Because it seems like the unit tests should fail with this line removed.

This is special handling of test code that is no longer needed, since we are not using mocha anymore (karma runs our browser tests). So the same test passes now without this special handling.

As for running the unit tests - all tests run as part of CI, and I usually run them locally as well. There are some tests that are "harder" to run locally, such as the minified integration tests (see /integration/firestore), but even those are run in CI.

dconeybe · 2020-03-30T13:39:48Z

packages/firestore/src/local/encoded_resource_path.ts

- * always terminated with a separator, and so a successor can always be
- * cheaply computed by incrementing the last character of the path.
- */
-export function prefixSuccessor(


Just for my own information, is it acceptable to delete exported functions as long as they are not "publicly exported" in https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts? Otherwise, wouldn't this be a backwards-incompatible change?

In general, if code is not in the api/ folder, it can't be used by our users.

In more technical terms, only code that is exported from our index.ts files is considered part of the public API:
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/register-module.ts

Furthermore, the files under /api should match our types (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts). All code in the exported classes above should either be part of the public API or prefixed with an underscore. We don't always get this right though - see https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/database.ts#L653. logLevel is not in our types, but can be accessed since it is exported as part of Firestore.

Does removing this actually help? It seems like this should have been tree-shaken out?

The Tree-Shaking in the browser builds doesn't actually work very well. I am trying to fix that (currently not working though - #2829), but for the "older builds" only excludes code that in modules that aren't loaded at all. Only the ESM2017 build will not include functions, but also only if they are deemed "side-effect free".

schmidt-sebastian

Thanks for your review!

schmidt-sebastian · 2020-03-30T16:20:06Z

packages/firestore/src/util/async_queue.ts

-          // Escape the promise chain and throw the error globally so that
-          // e.g. any global crash reporting library detects and reports it.
-          // (but not for simulated errors in our tests since this breaks mocha)
-          if (message.indexOf('Firestore Test Simulated Error') < 0) {


This is special handling of test code that is no longer needed, since we are not using mocha anymore (karma runs our browser tests). So the same test passes now without this special handling.

As for running the unit tests - all tests run as part of CI, and I usually run them locally as well. There are some tests that are "harder" to run locally, such as the minified integration tests (see /integration/firestore), but even those are run in CI.

schmidt-sebastian · 2020-03-30T16:26:12Z

packages/firestore/src/local/encoded_resource_path.ts

- * always terminated with a separator, and so a successor can always be
- * cheaply computed by incrementing the last character of the path.
- */
-export function prefixSuccessor(


In general, if code is not in the api/ folder, it can't be used by our users.

In more technical terms, only code that is exported from our index.ts files is considered part of the public API:
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/register-module.ts

Furthermore, the files under /api should match our types (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts). All code in the exported classes above should either be part of the public API or prefixed with an underscore. We don't always get this right though - see https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/database.ts#L653. logLevel is not in our types, but can be accessed since it is exported as part of Firestore.

schmidt-sebastian · 2020-03-30T16:26:54Z

@wilhuff for approval.

wilhuff · 2020-03-30T17:02:45Z

packages/firestore/src/local/encoded_resource_path.ts

- * always terminated with a separator, and so a successor can always be
- * cheaply computed by incrementing the last character of the path.
- */
-export function prefixSuccessor(


Does removing this actually help? It seems like this should have been tree-shaken out?

wilhuff · 2020-03-30T17:06:05Z

packages/firestore/src/local/target_data.ts

@@ -118,18 +118,4 @@ export class TargetData {
      this.resumeToken
    );
  }
-
-  isEqual(other: TargetData): boolean {


Isn't isEqual used indirectly by our custom deep equal mechanism?

I worry that we may have tests that are checking that something is not equal that are now accidentally passing while checking reference equality.

TargetData was never passed to our custom "equals" implementation. I actually looked back at the usage and since there is only one call, I decided to inline it. So now we have no code that treats objects with isEqual and without in the same codepath.

wilhuff

LGTM

Remove some unused code

fa16463

schmidt-sebastian force-pushed the mrschmidt/byebyes branch from a125161 to fa16463 Compare March 28, 2020 00:31

schmidt-sebastian requested review from wu-hui and dconeybe and removed request for wu-hui March 29, 2020 20:03

schmidt-sebastian assigned dconeybe Mar 29, 2020

schmidt-sebastian added the api: firestore label Mar 29, 2020

dconeybe approved these changes Mar 30, 2020

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Mar 30, 2020

schmidt-sebastian commented Mar 30, 2020

View reviewed changes

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Mar 30, 2020

wilhuff reviewed Mar 30, 2020

View reviewed changes

schmidt-sebastian added 2 commits March 30, 2020 15:28

Merge

e65ca11

Merge branch 'master' into mrschmidt/byebyes

c99dba1

schmidt-sebastian force-pushed the mrschmidt/byebyes branch from e39d46f to c99dba1 Compare March 30, 2020 22:30

Cleanup

4241537

wilhuff approved these changes Mar 31, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Mar 31, 2020

Merge

7fb6f2b

schmidt-sebastian merged commit e4b610d into master Mar 31, 2020

schmidt-sebastian deleted the mrschmidt/byebyes branch April 6, 2020 19:26

firebase locked and limited conversation to collaborators May 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove unused code #2817

Remove unused code #2817

Uh oh!

schmidt-sebastian commented Mar 28, 2020 •

edited

Loading

Uh oh!

google-oss-bot commented Mar 28, 2020 •

edited

Loading

Uh oh!

dconeybe Mar 30, 2020

Uh oh!

schmidt-sebastian Mar 30, 2020

Uh oh!

dconeybe Mar 30, 2020

Uh oh!

schmidt-sebastian Mar 30, 2020

Uh oh!

wilhuff Mar 30, 2020

Uh oh!

schmidt-sebastian Mar 30, 2020

Uh oh!

schmidt-sebastian left a comment

Uh oh!

schmidt-sebastian Mar 30, 2020

Uh oh!

schmidt-sebastian Mar 30, 2020

Uh oh!

schmidt-sebastian commented Mar 30, 2020

Uh oh!

wilhuff Mar 30, 2020

Uh oh!

wilhuff Mar 30, 2020

Uh oh!

schmidt-sebastian Mar 30, 2020

Uh oh!

wilhuff left a comment

Uh oh!

Uh oh!

Remove unused code #2817

Remove unused code #2817

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Binary Size Report

Affected SDKs

Test Logs

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 left a comment

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 Mar 30, 2020

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!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Mar 28, 2020 •

edited

Loading

google-oss-bot commented Mar 28, 2020 •

edited

Loading