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

Add test-only API shim (firestore-exp) #3361

Merged
merged 5 commits into from
Jul 9, 2020
Merged

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Jul 6, 2020

This PR adds the API shim that I am using to run the existing integration tests agains the tree-shakeable API. It essentially exposes the tree-shakeable client in terms of the old API.

The code is unused right now, but works as shown here: #3338

@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2020

🦋 Changeset is good to go

Latest commit: 1a0ce7e

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 schmidt-sebastian changed the base branch from master to mrschmidt/transactions July 6, 2020 22:03
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 6, 2020

Binary Size Report

Affected SDKs

  • @firebase/auth

    Type Base (417dd7c) Head (2254bcb) Diff
    main 177 kB 177 kB +137 B (+0.1%)
    module 177 kB 177 kB +137 B (+0.1%)
  • @firebase/firestore

    Type Base (417dd7c) Head (2254bcb) Diff
    browser 247 kB 247 kB +647 B (+0.3%)
    esm2017 194 kB 194 kB +251 B (+0.1%)
    main 494 kB 495 kB +1.17 kB (+0.2%)
    module 244 kB 245 kB +615 B (+0.3%)
    react-native 194 kB 194 kB +251 B (+0.1%)
  • @firebase/firestore/exp

    Type Base (417dd7c) Head (2254bcb) Diff
    main 399 kB 400 kB +506 B (+0.1%)
  • @firebase/firestore/memory

    Type Base (417dd7c) Head (2254bcb) Diff
    browser 184 kB 185 kB +333 B (+0.2%)
    esm2017 145 kB 145 kB +86 B (+0.1%)
    main 363 kB 364 kB +374 B (+0.1%)
    module 183 kB 183 kB +311 B (+0.2%)
    react-native 145 kB 145 kB +86 B (+0.1%)
  • firebase

    Type Base (417dd7c) Head (2254bcb) Diff
    firebase-auth.js 173 kB 174 kB +133 B (+0.1%)
    firebase-firestore.js 285 kB 285 kB +605 B (+0.2%)
    firebase-firestore.memory.js 224 kB 224 kB +310 B (+0.1%)
    firebase.js 818 kB 819 kB +741 B (+0.1%)

Test Logs

@schmidt-sebastian schmidt-sebastian changed the title Add test-only API shim Add test-only API shim (firestore-exp) Jul 7, 2020
@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/transactions to master July 8, 2020 16:57
Copy link

@thebrianchen thebrianchen left a comment

Choose a reason for hiding this comment

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

lgtm

}

/**
* Takes user data that uses API types from this shim and replaces them

Choose a reason for hiding this comment

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

ultra-uber-nit (so that I can say I left a comment): Move "with" to line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Sorry - I just realized I forgot to do this :/)

}

update(
documentRef: DocumentReference<any>,

Choose a reason for hiding this comment

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

Is there a reason why we're using <any> here as opposed to <unknown>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Any" and "Unknown" are different for TypeScript, and "any" encapsulates more variety. Since "any" in used in the .d.ts file, I have to use "any" here too. I wish that wasn't the case.

let options: legacy.SnapshotListenOptions = {};
let userObserver: PartialObserver<DocumentSnapshot<T>>;

if (isPartialObserver(args[0])) {

Choose a reason for hiding this comment

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

optional: There seems to be some code overlap between here and L547. You could probably extract the logic into a shared method, but might not be worth it considering it's just shim testing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Done.

}

export class FieldPath implements legacy.FieldPath {
private fieldNames: string[];

Choose a reason for hiding this comment

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

nit: (courtesy of intellij) s/private/private readonly

@schmidt-sebastian schmidt-sebastian merged commit 9b53ec8 into master Jul 9, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/expshim branch July 9, 2020 17:44
@firebase firebase locked and limited conversation to collaborators Aug 9, 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