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

feat: add support for Typescript Custom Mapping #828

Merged
merged 21 commits into from Jan 14, 2020
Merged

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented Dec 19, 2019

Fixes #856

Added withConverter() methods on CollectionReference, DocumentReference, and Query that allow developers to use Firestore classes with generics.

Example usage:

class Post {
  constructor(readonly title: string, readonly author: string) {}
  toString(): string {
    return this.title + ', by ' + this.author;
  }
}

const PostConverter = {
  toFirestore(post: Post): DocumentData {
    return {title: post.title, author: post.author};
  },
  fromFirestore(data: DocumentData): Post {
    return new Post(data.title, data.author);
  }
};

const postSnap = await firebase.firestore().collection('posts').withConverter(PostConverter).doc().get();
const post = postSnap.data();
if (post !== undefined) {
  post.title; // string
  post.toString(); // Should be defined with your overload
  post.someNonExistentProperty; // TS error
}

@thebrianchen thebrianchen self-assigned this Dec 19, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 19, 2019
@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a810aa5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #828   +/-   ##
=========================================
  Coverage          ?   96.58%           
=========================================
  Files             ?       25           
  Lines             ?    15418           
  Branches          ?     1137           
=========================================
  Hits              ?    14891           
  Misses            ?      519           
  Partials          ?        8

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a810aa5...6d4aebc. Read the comment docs.

@thebrianchen thebrianchen changed the title feat: generics (WIP) feat: Add generics support Jan 9, 2020
@thebrianchen thebrianchen changed the title feat: Add generics support feat: add generics support Jan 9, 2020
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thank you! I am super stoked that this coming together.

dev/src/document.ts Outdated Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
dev/src/index.ts Show resolved Hide resolved
dev/src/document.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/test/util/helpers.ts Outdated Show resolved Hide resolved
dev/test/watch.ts Show resolved Hide resolved
types/firestore.d.ts Show resolved Hide resolved
types/firestore.d.ts Show resolved Hide resolved
types/firestore.d.ts Show resolved Hide resolved
@thebrianchen thebrianchen removed their assignment Jan 10, 2020
dev/src/document.ts Outdated Show resolved Hide resolved
ref: DocumentReference,
fieldsProto?: ApiMapValue,
ref: DocumentReference<T>,
readonly fieldsProto?: ApiMapValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am undecided whether this should be fieldsProto or _ fieldsProto. This is a public field for internal consumption.

Copy link
Author

Choose a reason for hiding this comment

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

I've renamed to _fieldsProto so that external developers know that it's not intended for external consumption.

dev/src/index.ts Outdated Show resolved Hide resolved
dev/src/transaction.ts Show resolved Hide resolved
dev/src/watch.ts Show resolved Hide resolved
dev/src/watch.ts Show resolved Hide resolved
dev/test/collection.ts Show resolved Hide resolved
dev/test/collection.ts Show resolved Hide resolved
dev/test/typescript.ts Outdated Show resolved Hide resolved
Copy link
Author

@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.

thanks for the edits and trying out the queryOptions + stream stuff on your own!

dev/src/document.ts Outdated Show resolved Hide resolved
ref: DocumentReference,
fieldsProto?: ApiMapValue,
ref: DocumentReference<T>,
readonly fieldsProto?: ApiMapValue,
Copy link
Author

Choose a reason for hiding this comment

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

I've renamed to _fieldsProto so that external developers know that it's not intended for external consumption.

dev/src/watch.ts Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
dev/test/collection.ts Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/test/typescript.ts Outdated Show resolved Hide resolved
dev/src/transaction.ts Show resolved Hide resolved
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Some final comments, but looks good. We should probably change the title to something less generic... 'generics' are the tool we used to implement withConverter, but it does not describe the feature very well.

dev/src/reference.ts Show resolved Hide resolved
dev/src/types.ts Outdated Show resolved Hide resolved
dev/src/types.ts Show resolved Hide resolved
dev/test/document.ts Show resolved Hide resolved
dev/test/document.ts Outdated Show resolved Hide resolved
dev/test/typescript.ts Outdated Show resolved Hide resolved
@schmidt-sebastian
Copy link
Contributor

schmidt-sebastian commented Jan 14, 2020

Can you also make the description of the PR a bit more expressive so that users can use it as a guide for how this feature works?

@thebrianchen thebrianchen changed the title feat: add generics support feat: add support for Typescript Custom Mapping Jan 14, 2020
@thebrianchen thebrianchen merged commit 94ddc89 into master Jan 14, 2020
@thebrianchen thebrianchen deleted the bc/generics branch January 14, 2020 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port FirebaseDataConverter interface
4 participants