Skip to content
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.

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

Remove null and NaN checks in filters #3960

Merged
merged 5 commits into from
Oct 20, 2020
Merged

Conversation

thebrianchen
Copy link

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2020

🦋 Changeset detected

Latest commit: efd4ed1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@firebase/firestore Minor
firebase Patch
@firebase/rules-unit-testing Patch

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 16, 2020

Binary Size Report

Affected SDKs

  • @firebase/database

    Type Base (4b540f9) Head (038dc2c) Diff
    browser 269 kB 270 kB +1.04 kB (+0.4%)
    esm2017 237 kB 238 kB +841 B (+0.4%)
    main 271 kB 272 kB +1.04 kB (+0.4%)
    module 269 kB 270 kB +1.04 kB (+0.4%)
  • @firebase/firestore

    Type Base (4b540f9) Head (038dc2c) Diff
    browser 239 kB 238 kB -658 B (-0.3%)
    esm2017 190 kB 190 kB -641 B (-0.3%)
    main 471 kB 470 kB -849 B (-0.2%)
    module 239 kB 238 kB -658 B (-0.3%)
    react-native 190 kB 190 kB -641 B (-0.3%)
  • @firebase/firestore/exp

    Type Base (4b540f9) Head (038dc2c) Diff
    browser 188 kB 188 kB -641 B (-0.3%)
    main 476 kB 475 kB -849 B (-0.2%)
    module 188 kB 188 kB -641 B (-0.3%)
    react-native 189 kB 188 kB -641 B (-0.3%)
  • @firebase/firestore/lite

    Type Base (4b540f9) Head (038dc2c) Diff
    browser 62.2 kB 61.6 kB -573 B (-0.9%)
    main 137 kB 137 kB -849 B (-0.6%)
    module 62.2 kB 61.6 kB -573 B (-0.9%)
    react-native 62.4 kB 61.8 kB -573 B (-0.9%)
  • @firebase/firestore/memory

    Type Base (4b540f9) Head (038dc2c) Diff
    browser 177 kB 176 kB -658 B (-0.4%)
    esm2017 140 kB 140 kB -641 B (-0.5%)
    main 343 kB 342 kB -849 B (-0.2%)
    module 177 kB 176 kB -658 B (-0.4%)
    react-native 140 kB 140 kB -641 B (-0.5%)
  • firebase

    Type Base (4b540f9) Head (038dc2c) Diff
    firebase-database.js 189 kB 190 kB +1.00 kB (+0.5%)
    firebase-firestore.js 278 kB 277 kB -666 B (-0.2%)
    firebase-firestore.memory.js 217 kB 217 kB -666 B (-0.3%)
    firebase.js 823 kB 823 kB +336 B (+0.0%)

Test Logs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 16, 2020

Size Analysis Report

Affected Products

No changes between base commit (4b540f9) and head commit (038dc2c).

Test Logs

@thebrianchen thebrianchen assigned wilhuff and unassigned thebrianchen Oct 19, 2020
@thebrianchen thebrianchen requested a review from wilhuff October 19, 2020 16:10
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

'@firebase/firestore': minor
---

Removed validation of `null` and `NaN` values in filters. However, note that only `==` and `!=` filters match against `null` and `NaN`.
Copy link
Contributor

Choose a reason for hiding this comment

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

However, note that only == and != filters match against null and NaN.

This reads to me in a way that suggests we're removing guard rails but still expecting users to remember this. I'd suggest something more like this:

Removed excess validation of null and NaN values in query filters. This more closely aligns the SDK with the Firestore backend, which has always accepted null and NaN for all operators, even though this isn't necessarily useful.

Copy link
Author

Choose a reason for hiding this comment

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

done.

@@ -800,6 +813,14 @@ apiDescribe('Queries', (persistence: boolean) => {
// With objects.
const snapshot2 = await coll.where('zip', 'in', [{ code: 500 }]).get();
expect(toDataArray(snapshot2)).to.deep.equal([{ zip: { code: 500 } }]);

// With null.
const snapshot3 = await coll.where('zip', 'in', [null]).get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test that shows that "in" with null/nan and something else still finds the something else?

Copy link
Author

Choose a reason for hiding this comment

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

done.


// With null.
const snapshot3 = await coll
.where('zip', 'array-contains-any', [null])
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test that shows that "array-contains-any" with null/nan and something else still finds the something else?

Copy link
Author

Choose a reason for hiding this comment

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

done.

@wilhuff wilhuff assigned thebrianchen and unassigned wilhuff Oct 19, 2020
@thebrianchen thebrianchen merged commit 9719635 into master Oct 20, 2020
@thebrianchen thebrianchen deleted the bc/remove-null-check branch October 20, 2020 21:47
@google-oss-bot google-oss-bot mentioned this pull request Oct 22, 2020
@firebase firebase locked and limited conversation to collaborators Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants