Skip to content

Cater for shapes where keys are not all strings in AddPrefixToKeys interface definition#6371

Open
nwaughachukwuma wants to merge 1 commit intofirebase:mainfrom
nwaughachukwuma:refactor-add-prefix-to-keys-interface
Open

Cater for shapes where keys are not all strings in AddPrefixToKeys interface definition#6371
nwaughachukwuma wants to merge 1 commit intofirebase:mainfrom
nwaughachukwuma:refactor-add-prefix-to-keys-interface

Conversation

@nwaughachukwuma
Copy link
Copy Markdown

@nwaughachukwuma nwaughachukwuma commented Jun 21, 2022

Summary

Fixes #6105

The issue comes from AddPrefixToKeys interface, shown below, in use cases where nested interfaces are defined as Record<string, Model> or {[key: string]: Model} - it looks like Typescript has a challenge inferring such shape as it considers K to be of type string|number|symbol.

export declare type AddPrefixToKeys<Prefix extends string, T extends Record<string, unknown>> = {
  [K in keyof T & string as`${Prefix}.${K}`]+?: T[K];
};

To solve this, we have to tell Typescript to infer the type or use a conditional statement to determine the type as shown below. Now, typescript can infer K and know its shape n-levels deep.

export declare type AddPrefixToKeys<Prefix extends string, T extends Record<string, unknown>> = {
  [K in keyof T as K extends string ? `${Prefix}.${K}`: K]+?: T[K];
};

This Typescript playground shows what's happening - all interface definitions come from the current version of firebase-js-sdk (firebase v9.7.0). I've commented the current logic for the fix. Switch between the 2 to see how the fix solves the issue

cc @ehsannas

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 21, 2022

⚠️ No Changeset found

Latest commit: 722ed36

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

@ehsannas
Copy link
Copy Markdown
Contributor

Thanks @nwaughachukwuma for your contribution, and apologies that this had fallen off my task list.

@felixgabler
Copy link
Copy Markdown

Hi! Is anyone working on this? Would be really awesome to fix.

@ehsannas ehsannas requested a review from MarkDuckworth October 5, 2022 16:49
@MarkDuckworth
Copy link
Copy Markdown
Contributor

@nwaughachukwuma, apologies for the long delay on this. I've been looking at this recently and I'd like to accept the change, but I'm having trouble getting this solution to work. In the Typescript playground sample you linked, you can see the error when assigning to q on line 29. Still looking for a fix.

@nwaughachukwuma
Copy link
Copy Markdown
Author

Hi @MarkDuckworth, it's been a while but let me take another look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with type UpdateData<T> from firebase/firestore

4 participants