Tree-shakeable FieldFilter

[schmidt-sebastian]

This PR makes the Proto-consuming code in FieldFilter tree-shakeable. Once this is merged, we can make Query tree-shakeable.

[google-oss-bot]

Binary Size Report

Affected SDKs

• @firebase/firestore

  Type	Base (f54abc2)	Head (220b040)	Diff
  browser	248 kB	248 kB	-79 B (-0.0%)
  esm2017	194 kB	194 kB	-24 B (-0.0%)
  main	493 kB	493 kB	+190 B (+0.0%)
  module	246 kB	246 kB	-79 B (-0.0%)

• @firebase/firestore/exp

  Type	Base (f54abc2)	Head (220b040)	Diff
  main	282 kB	282 kB	+267 B (+0.1%)

• @firebase/firestore/lite

  Type	Base (f54abc2)	Head (220b040)	Diff
  main	141 kB	141 kB	+267 B (+0.2%)

• @firebase/firestore/memory

  Type	Base (f54abc2)	Head (220b040)	Diff
  browser	186 kB	186 kB	-79 B (-0.0%)
  esm2017	145 kB	145 kB	-24 B (-0.0%)
  main	363 kB	363 kB	+190 B (+0.1%)
  module	184 kB	184 kB	-79 B (-0.0%)

• firebase

  Type	Base (f54abc2)	Head (220b040)	Diff
  firebase-firestore.js	286 kB	286 kB	-103 B (-0.0%)
  firebase-firestore.memory.js	226 kB	226 kB	-103 B (-0.0%)
  firebase.js	820 kB	819 kB	-103 B (-0.0%)

Test Logs

• Base (f54abc25): https://github.com/firebase/firebase-js-sdk/actions/runs/152278562
• Head (220b040b): https://github.com/firebase/firebase-js-sdk/actions/runs/153034565

[wilhuff]

Could you explain how this results in any net savings for users?

AFAICT, you've changed this from a set of classes with polymorphic behavior to a big if-then ladder, but so long as you run queries at all, you still get all the code. There's been a very slight savings in package size at a massive cost in readability and maintainability.

FWIW, the other kind of filter we had mind were conjunctions and disjunctions.

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 9a26ff6

We got this.

This PR includes changesets to release 0 packages

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

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

[schmidt-sebastian]

Sorry, I missed your comment. The idea was to make both FieldFilter.equals() and FieldFilter.canonicalId() optional, which leads to FieldFilter classes that only have a single method ("matches()"). This, combined with the existing if-then code in FieldFilter.create() led me down this path. I'll explore adding the classes back, but they will only have this one method.

[schmidt-sebastian]

Added back the FieldFilter classes, but they only support matches(). canonifyFilter, filterEquals and stringifyFilter are the remaining size savings (that will only come into play once Query is updated as well).

[wilhuff]

Basically LGTM with a style question.

[wilhuff]

I think we've discussed this before, so I won't push, but I find the difference in ordering (subject-operation vs operation-subject) between orderByEquals and stringifyOrderBy to be jarring. Autocompletion and muscle memory would be much easier if there was just one way to do it.

If these were all in the form subject-operation they would group nicely in imports as well.

What do you think of these?

orderByEquals
orderByToString
orderByCanonicalId

[schmidt-sebastian]

The naming in this PR is inspired by the names in the Values helper. I am also not a big fan how "equals" is an outlier, but on its own I do think that each name looks ok. For that reason, I am kind of reluctant to change it right now, but I am not opposed to overhauling all stringify/canonicalize methods in a follow up.

[wilhuff]

LGTM