Skip to content

Added support for Timestamp and DocumentReference #28

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

Conversation

the0rem
Copy link
Contributor

@the0rem the0rem commented Oct 24, 2018

Closes #24

This PR is to add serialisation support for firestore.Timestamp and DocumentReference for use in makeDocumentSnapshot.

This fills in the gaps of functionality for the following:

  • Online tests can now be written for Firestore functions that need to receive a DocumentReference as part of the DocumentSnapshot
  • firestore.Timestamp can now be used instead of just Date. This is very useful when defining interfaces and models in TS where setting a timestamp had to use Date, but when getting the timestamp from the database would be firestore.Timestamp;

Tests are passing however I haven't added any new tests; There's currently no coverage for this part of the package.

@laurenzlong
Copy link
Contributor

What was the test case you used to manually test this? Can you provide some sample code?

@laurenzlong laurenzlong self-requested a review October 24, 2018 17:58
@laurenzlong
Copy link
Contributor

Also, thank you for making this PR!

@the0rem
Copy link
Contributor Author

the0rem commented Oct 25, 2018

I've simplified and obfuscated our code to produce the example however the method remains the same:

const test: FeaturesList = _test(appOptions, serviceKey)
const testFunction: WrappedFunction = test.wrap(someCloudFunction);
const subRecordRef: DocumentReference = admin.firestore().doc(appointmentPath);

const record: IRecord = mockRecord({
  ref: subRecordRef,
  date: Timestamp.fromDate(moment().toDate()),
});

const beforeSnap: DocumentSnapshot = test.firestore.makeDocumentSnapshot({}, path);
const afterSnap: DocumentSnapshot = test.firestore.makeDocumentSnapshot(record, path);
const change: Change<DocumentSnapshot> = test.makeChange(beforeSnap, afterSnap);

await testFunction(change, { params: testParams });

This PR changes are being used internally which is working a treat :)

Copy link
Contributor

@laurenzlong laurenzlong left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! I was at Firebase Summit. I made some suggestions to the code.

@the0rem
Copy link
Contributor Author

the0rem commented Nov 2, 2018

Thanks for the feedback @laurenzlong. Those changes have been made and are passing our internal tests.

@the0rem
Copy link
Contributor Author

the0rem commented Nov 2, 2018

Looks like travis failed for unrelated reasons.

@the0rem the0rem closed this Nov 2, 2018
@the0rem the0rem reopened this Nov 2, 2018
@laurenzlong
Copy link
Contributor

Looks good! Seems like Travis is failing because a dependency is failing for Node 11. I'll resolve that separately, before merging this PR. Thanks!

@the0rem
Copy link
Contributor Author

the0rem commented Nov 13, 2018

Hi @laurenzlong any update on this?

@laurenzlong laurenzlong merged commit 318625b into firebase:master Nov 17, 2018
@laurenzlong
Copy link
Contributor

Sorry for the delay!

@laurenzlong laurenzlong mentioned this pull request Nov 19, 2018
@laurenzlong
Copy link
Contributor

This is now a part of v0.1.5

@the0rem the0rem deleted the feature/timestamp-and-reference-serialisation branch September 29, 2021 08:07
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.

2 participants