Skip to content

Add set() overrides to lite sdk #3291

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 23 commits into from
Jun 27, 2020
Merged

Add set() overrides to lite sdk #3291

merged 23 commits into from
Jun 27, 2020

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented Jun 25, 2020 •
edited
Loading

Setting the base as #3254 since it requires the changes to applyFirestoreDataConverter. Since this branch sits on top of #3254, I will diff this against master once #3254 goes in.

@thebrianchen thebrianchen self-assigned this Jun 25, 2020
@changeset-bot
Copy link

changeset-bot bot commented Jun 25, 2020 •
edited
Loading

🦋 Changeset is good to go

Latest commit: 4323e7e

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 25, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (39ca8ec) Head (738f429) Diff
    browser 248 kB 249 kB +226 B (+0.1%)
    esm2017 194 kB 194 kB +180 B (+0.1%)
    main 494 kB 494 kB +496 B (+0.1%)
    module 246 kB 246 kB +226 B (+0.1%)

  • @firebase/firestore/exp

    Type Base (39ca8ec) Head (738f429) Diff
    main 37.5 kB 272 kB +235 kB (+625.5%)

  • @firebase/firestore/lite

    Type Base (39ca8ec) Head (738f429) Diff
    main 140 kB 141 kB +179 B (+0.1%)

  • @firebase/firestore/memory

    Type Base (39ca8ec) Head (738f429) Diff
    browser 186 kB 186 kB +226 B (+0.1%)
    esm2017 145 kB 145 kB +180 B (+0.1%)
    main 363 kB 364 kB +496 B (+0.1%)
    module 184 kB 185 kB +226 B (+0.1%)

  • firebase

    Type Base (39ca8ec) Head (738f429) Diff
    firebase-firestore.js 287 kB 287 kB +216 B (+0.1%)
    firebase-firestore.memory.js 226 kB 226 kB +216 B (+0.1%)
    firebase.js 820 kB 820 kB +216 B (+0.0%)

Test Logs

@thebrianchen thebrianchen force-pushed the bc/lite-set branch 2 times, most recently from e1a25a0 to 9d8219b Compare June 26, 2020 04:54
schmidt-sebastian
schmidt-sebastian reviewed Jun 26, 2020
View reviewed changes
packages/firestore/lite/index.d.ts Outdated
@@ -45,6 +45,7 @@ export function setLogLevel(logLevel: LogLevel): void;

export interface FirestoreDataConverter<T> {
toFirestore(modelObject: T): DocumentData;
toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;
fromFirestore(snapshot: QueryDocumentSnapshot): T;
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should we explicitly type this as QueryDocumentSnapshot<DocumentData> to make it easier to understand?

Copy link
Author

Choose a reason for hiding this comment

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

done.

packages/firestore/lite/test/integration.test.ts Outdated
});
});

it('WriteBatch.set() supports partials with merge', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move these tests to https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/lite/test/integration.test.ts#L253, which allows you to write one test for WriteBatch, Transaction and set().

Copy link
Author

Choose a reason for hiding this comment

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

done.

packages/firestore/lite/index.d.ts
@@ -45,6 +45,7 @@ export function setLogLevel(logLevel: LogLevel): void;

export interface FirestoreDataConverter<T> {
toFirestore(modelObject: T): DocumentData;
toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this change also need to be made in exp/index.d.ts

Copy link
Author

Choose a reason for hiding this comment

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

done for converter and SetOptions.

Base automatically changed from bc/set-override to master June 26, 2020 19:39
@thebrianchen thebrianchen merged commit af0c430 into master Jun 27, 2020
@thebrianchen thebrianchen deleted the bc/lite-set branch June 27, 2020 18:37
schmidt-sebastian pushed a commit that referenced this pull request Jun 30, 2020
@schmidt-sebastian
Add set() overrides to lite sdk (#3291)
4b8b439
@firebase firebase locked and limited conversation to collaborators Jul 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@schmidt-sebastian schmidt-sebastian schmidt-sebastian approved these changes

@alikn alikn Awaiting requested review from alikn

@andirayo andirayo Awaiting requested review from andirayo

@avolkovi avolkovi Awaiting requested review from avolkovi

@egilmorez egilmorez Awaiting requested review from egilmorez

@erikeldridge erikeldridge Awaiting requested review from erikeldridge

@sam-gc sam-gc Awaiting requested review from sam-gc

@scottcrossen scottcrossen Awaiting requested review from scottcrossen

@yuchenshi yuchenshi Awaiting requested review from yuchenshi

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

@zwu52 zwu52 Awaiting requested review from zwu52

Assignees

@thebrianchen thebrianchen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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