Skip to content

New Query API #3390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Jul 21, 2020
Merged

New Query API #3390

merged 10 commits into from
Jul 21, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Jul 10, 2020 •
edited
Loading

This is the new Query API proposed in go/firestore-next-amendment

Couple notes:

  • I split up the Query class into Query and QueryImpl (similar to Target and TargetImpl) to not expose memoizedOrderBy and related fields as a public field.
  • There shouldn't be any logic changes within the individual methods that I made tree-shakeable.
  • The diff seems much easier to review in IntelliJ than in GitHub.
  • I did not (yet) update the dependency files to keep the PR "small"
  • There is no rush with this PR since the API won't be reviewed until July 23rd.

@changeset-bot
Copy link

changeset-bot bot commented Jul 10, 2020 •
edited
Loading

💥 No Changeset

Latest commit: 0de9472

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, 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
Copy link
Contributor

google-oss-bot commented Jul 10, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (cf8696e) Head (9a83673) Diff
    browser 246 kB 245 kB -587 B (-0.2%)
    esm2017 193 kB 193 kB -43 B (-0.0%)
    main 469 kB 469 kB +395 B (+0.1%)
    module 244 kB 243 kB -578 B (-0.2%)
    react-native 193 kB 193 kB -44 B (-0.0%)

  • @firebase/firestore/exp

    Type Base (cf8696e) Head (9a83673) Diff
    browser 187 kB 187 kB +344 B (+0.2%)
    main 508 kB 511 kB +2.56 kB (+0.5%)
    react-native 187 kB 187 kB +324 B (+0.2%)

  • @firebase/firestore/lite

    Type Base (cf8696e) Head (9a83673) Diff
    browser 67.9 kB 68.0 kB +132 B (+0.2%)
    main 494 kB 497 kB +2.56 kB (+0.5%)
    react-native 67.9 kB 68.1 kB +132 B (+0.2%)

  • @firebase/firestore/memory

    Type Base (cf8696e) Head (9a83673) Diff
    browser 185 kB 184 kB -612 B (-0.3%)
    esm2017 145 kB 145 kB -70 B (-0.0%)
    main 344 kB 344 kB +187 B (+0.1%)
    module 183 kB 182 kB -602 B (-0.3%)
    react-native 145 kB 145 kB -70 B (-0.0%)

  • firebase

    Type Base (cf8696e) Head (9a83673) Diff
    firebase-firestore.js 284 kB 283 kB -642 B (-0.2%)
    firebase-firestore.memory.js 224 kB 224 kB -672 B (-0.3%)
    firebase.js 818 kB 817 kB -643 B (-0.1%)

Test Logs

@schmidt-sebastian schmidt-sebastian changed the title WIP New Query API New Query API Jul 13, 2020
@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt/modularquery branch 2 times, most recently from 17b6d3d to 7e81537 Compare July 15, 2020 00:59
@schmidt-sebastian schmidt-sebastian mentioned this pull request Jul 15, 2020
Merged
@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt/modularquery branch 2 times, most recently from bde4f39 to 7958de9 Compare July 15, 2020 17:15
dconeybe
dconeybe approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/firestore/exp/test/shim.ts Outdated
@@ -54,7 +55,15 @@ import {
terminate,
updateDoc,
waitForPendingWrites,
writeBatch
writeBatch,
limit,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these imports be ordered alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to defer this to the day when we have a lint rule for this. We have tried to keep imports sorted, but it is hard to maintain without tooling. I have since given up on this completely and life has been fabulous :)

packages/firestore/exp/test/shim.ts Outdated
@@ -265,7 +274,9 @@ export class DocumentReference<T = legacy.DocumentData>
readonly path = this._delegate.path;

get parent(): legacy.CollectionReference<T> {
return new CollectionReference<T>(parent(this._delegate));
// TODO(firestoreexp): This bang should be removed one CollectionReference
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"bang"? Is this TODO obsolete and can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed (last week, but well after the PR was opened).

See also here for explanation: https://stackoverflow.com/questions/42273853/in-typescript-what-is-the-exclamation-mark-bang-operator-when-dereferenci

packages/firestore/lite/index.node.ts Outdated
@@ -42,7 +43,16 @@ export {
updateDoc,
addDoc,
refEqual,
queryEqual
queryEqual,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here... are imports typically sorted alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Summary: Yes, but no :)

packages/firestore/lite/test/shim.ts Outdated
@@ -246,7 +255,9 @@ export class DocumentReference<T = legacy.DocumentData>
readonly path = this._delegate.path;

get parent(): legacy.CollectionReference<T> {
return new CollectionReference<T>(parent(this._delegate));
// TODO(firestorelite): This bang should be removed one CollectionReference
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment about a "bang" still relevant? I'm not sure what it means in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of #3402, TypeScript can now tell our DocumentReferences/CollectionReferences apart. Bang has been removed. It was needed to take the nullability away from one the invalid parent overloads.

@schmidt-sebastian schmidt-sebastian merged commit eb44f4a into master Jul 21, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/modularquery branch July 21, 2020 03:33
@firebase firebase locked and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dconeybe dconeybe dconeybe approved these changes

Assignees

@schmidt-sebastian schmidt-sebastian

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schmidt-sebastian @google-oss-bot @dconeybe