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.

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

Make FieldValue a Compat class #3819

Merged
merged 6 commits into from
Sep 23, 2020
Merged

Make FieldValue a Compat class #3819

merged 6 commits into from
Sep 23, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Sep 21, 2020 •
edited
Loading

This is the first of many PRs for go/firebase-next-compat

The idea is that we will remove our legacy API types and instead replace them with shims that delegate all work to the new APIs from firestore-exp. We already use this "trick" in our tests: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/exp/test/shim.ts

This PR makes the FieldValue Shim in the test production ready:

  • It adds the "unwrap" code form the Shim (the code that extracts the firestore-exp type) into UserDataConverter. This allows the user to pass in both old and new types as Firestore values.
  • It adds API-level input validation to the FieldValue shim. The firestore-exp types are meant to be used from TypeScript, so we will no longer validate JS input.
  • Changes the API validation tests to remove references to the API names that are now hidden by the new shim.

@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2020 •
edited
Loading

⚠️ No Changeset found

Latest commit: acf89c7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, 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 Sep 21, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (70fe239) Head (c2f71d6) Diff
    browser 249 kB 249 kB +307 B (+0.1%)
    esm2017 196 kB 196 kB +155 B (+0.1%)
    main 483 kB 483 kB +336 B (+0.1%)
    module 246 kB 246 kB +317 B (+0.1%)
    react-native 196 kB 196 kB +155 B (+0.1%)

  • @firebase/firestore/exp

    Type Base (70fe239) Head (c2f71d6) Diff
    browser 189 kB 189 kB -212 B (-0.1%)
    main 478 kB 477 kB -901 B (-0.2%)
    module 189 kB 189 kB -212 B (-0.1%)
    react-native 189 kB 189 kB -212 B (-0.1%)

  • @firebase/firestore/lite

    Type Base (70fe239) Head (c2f71d6) Diff
    browser 63.8 kB 63.6 kB -212 B (-0.3%)
    main 141 kB 140 kB -901 B (-0.6%)
    module 63.8 kB 63.6 kB -212 B (-0.3%)
    react-native 64.1 kB 63.9 kB -212 B (-0.3%)

  • @firebase/firestore/memory

    Type Base (70fe239) Head (c2f71d6) Diff
    browser 186 kB 187 kB +307 B (+0.2%)
    esm2017 147 kB 147 kB +155 B (+0.1%)
    main 356 kB 357 kB +336 B (+0.1%)
    module 184 kB 185 kB +317 B (+0.2%)
    react-native 147 kB 147 kB +155 B (+0.1%)

  • @firebase/performance-exp

    Type Base (70fe239) Head (c2f71d6) Diff
    browser 27.3 kB 28.8 kB +1.43 kB (+5.2%)
    esm2017 25.5 kB 26.9 kB +1.44 kB (+5.6%)
    main 27.3 kB 28.8 kB +1.43 kB (+5.2%)
    module 27.2 kB 28.6 kB +1.42 kB (+5.2%)

  • firebase

    Type Base (70fe239) Head (c2f71d6) Diff
    firebase-firestore.js 287 kB 287 kB +313 B (+0.1%)
    firebase-firestore.memory.js 226 kB 227 kB +313 B (+0.1%)
    firebase.js 830 kB 830 kB +312 B (+0.0%)

Test Logs

@schmidt-sebastian schmidt-sebastian requested a review from a team as a code owner September 21, 2020 22:37
dconeybe
dconeybe approved these changes Sep 23, 2020
View reviewed changes
packages/firestore/src/compat/field_value.ts Outdated
}

isEqual(other: FieldValue): boolean {
return this._delegate === other._delegate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the implementation delegate to the isEqual method of this._delegate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I was under the impression that we didn't have an isEqual in the firestore-exp SDK. This is however part of the approved API, so let me see if I can add it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (also managed to remove SerializableFieldValue)

packages/firestore/test/integration/api/array_transforms.test.ts
@@ -144,6 +147,13 @@ apiDescribe('Array Transforms:', (persistence: boolean) => {
});
});

it('arrayUnion() supports DocumentReference', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this just some missing test coverage?

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 was afraid that this PR would break this behavior, since I wasn't sure if the new FieldValue types could deal with the legacy API.

packages/firestore/exp/test/shim.ts Outdated
constructor(
private readonly _firestore: FirebaseFirestore,
private readonly _delegate: exp.Transaction
) {}
private readonly delegate: exp.Transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the private and readonly modifiers on delegate be dropped? This may be my TypeScript noobyness surfacing, but wouldn't this create two properties, delegate and _delegate, both of which store the same exp.Transaction object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

s/noobyness/expertyness/

Thanks for catching.

@schmidt-sebastian schmidt-sebastian merged commit 0c607c9 into master Sep 23, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/compat-fieldvalue branch September 23, 2020 19:06
hsubox76 pushed a commit that referenced this pull request Sep 29, 2020
@schmidt-sebastian
Fix compile error + add Changelog for #3819 (#3864)
2be43ea
@schmidt-sebastian schmidt-sebastian changed the title Make FieldPath a Compat class Make FieldValue a Compat class Sep 30, 2020
@schmidt-sebastian schmidt-sebastian mentioned this pull request Sep 30, 2020
Closed
@firebase firebase locked and limited conversation to collaborators Oct 24, 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.

None yet
3 participants
@schmidt-sebastian @google-oss-bot @dconeybe