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

Changing UpdateData<T> to expand support for types with index signatures. #7318

Merged
merged 14 commits into from
Jun 30, 2023

Conversation

MarkDuckworth
Copy link
Contributor

@MarkDuckworth MarkDuckworth commented May 22, 2023

This change relaxes UpdateData<T>, so that dot notation can be used to update types T where T contains an index signature. An example type T that this affects:

interface MyV10ServerType {
  indexed: {
    [name: string]: {
      booleanProperty: boolean,
      numberProperty: number
    }
  }
};

Without this change, dot notation can not be used to update field 'indexed.foo.booleanProperty'.

const update: UpdateData<MyV10ServerType> = {
  'indexed.foo.booleanProperty': true <-- TS2322: Type 'true' is not assignable to type '{ booleanProperty?: boolean | FieldValue | undefined; numberProperty?: number | FieldValue | undefined; } | FieldValue | undefined'.
}

updateDoc<MyV10ServerType>(docRef, update);

With this change, the following code is now possible:

const update: UpdateData<MyV10ServerType> = {
  'indexed.foo.booleanProperty': true
}

updateDoc<MyV10ServerType>(docRef, update);

The downside of this change is that we lose type safety and auto-complete for these types. For example we can do the following:

const update: UpdateData<MyV10ServerType> = {
  'indexed.foo': 'foo is not a string, it should be an object, but we can't enforce that...'
}

However, type safety and auto-complete are still in place if doing full object replacement:

// This is an example of update data that does a full object replacement
const update: UpdateData<MyV10ServerType> = {
  indexed: {
    foo: {
      booleanProperty: true,
      numberProperty: 1
    }
  }
}

And, we should note that type safety and auto complete are still in place for nested types when index signature is not involved.

interface NestedObject {
  indexed: {
    // see, no index signature on this line
    foo: {
      booleanProperty: boolean,
      numberProperty: number
    }
  }
};

const update: UpdateData<NestedObject> = {
  'indexed.foo.booleanProperty': true // we have type safety and auto-complete for this
}

Fixes #6105

Reviewers should understand that the unit tests in lite-api/types.test.ts are all new. These tests are introduced in a different pull request (7319) and are intended to ensure that UpdateData does not regress in unexpected ways with the fix in this PR.

@MarkDuckworth MarkDuckworth requested review from a team as code owners May 22, 2023 23:08
@changeset-bot
Copy link

changeset-bot bot commented May 22, 2023

🦋 Changeset detected

Latest commit: 4ec7437

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

This PR includes changesets to release 3 packages
Name Type
@firebase/firestore Minor
firebase Patch
@firebase/firestore-compat 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 May 22, 2023

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 22, 2023

@MarkDuckworth MarkDuckworth changed the title Breaking changes for UpdateData to support types with index signatures. Changing UpdateData<T> to support types with index signatures. May 24, 2023
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.

Thanks for this fix! I can't honestly say I completely understand it; however, the I understand the unit tests so it looks good to me!

@dconeybe dconeybe assigned MarkDuckworth and unassigned dconeybe May 25, 2023
Base automatically changed from markduckworth/nested-update-data-v9 to master June 30, 2023 13:51
Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Looks good, just needs a changeset (minor)

@MarkDuckworth MarkDuckworth requested a review from a team as a code owner June 30, 2023 18:09
@MarkDuckworth MarkDuckworth changed the title Changing UpdateData<T> to support types with index signatures. Fixing UpdateData<T> to support types with index signatures. Jun 30, 2023
@MarkDuckworth MarkDuckworth changed the title Fixing UpdateData<T> to support types with index signatures. Changing UpdateData<T> to expand support for types with index signatures. Jun 30, 2023
@MarkDuckworth MarkDuckworth merged commit 57f2a86 into master Jun 30, 2023
26 checks passed
@MarkDuckworth MarkDuckworth deleted the markduckworth/nested-update-data-v10 branch June 30, 2023 21:34
@google-oss-bot google-oss-bot mentioned this pull request Jul 6, 2023
@dconeybe
Copy link
Contributor

dconeybe commented Jul 7, 2023

This was included in the v10.0.0 release: https://firebase.google.com/support/release-notes/js#version_1000_-_july_6_2023

@firebase firebase locked and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with type UpdateData<T> from firebase/firestore
6 participants