Add FieldValue methods to the Lite SDK #3135
Conversation
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/fieldvalue
branch
3 times, most recently
from
e50a76f to
ae95648
Compare
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/fieldvalue
branch
from
ae95648 to
a957d31
Compare
|
@dconeybe I think this is now ready for review (sorry for assigning this earlier). This ended up being a lot tricker (read: uglier) that I had hoped for. Let me know if you need help reviewing/want to send this somewhere else. |
Binary Size ReportAffected SDKs
Test Logs |
| export function arrayUnion(...elements: unknown[]): firestore.FieldValue { | ||
| validateAtLeastNumberOfArgs('arrayUnion()', arguments, 1); | ||
| // NOTE: We don't actually parse the data until it's used in set() or | ||
| // update() since we need access to the Firestore instance. |
There was a problem hiding this comment.
Consider tweaking this part of the sentence to read: "since we'd need the Firestore instance to do this." (also on line 87).
There was a problem hiding this comment.
Done (4x times)
| expectEqual(FieldValue.delete(), FieldValue.delete()); | ||
| expectEqual(FieldValue.serverTimestamp(), FieldValue.serverTimestamp()); | ||
| expectNotEqual(FieldValue.delete(), FieldValue.serverTimestamp()); | ||
| }); |
There was a problem hiding this comment.
Add a test case to exercise the if (!(other instanceof FieldValueDelegate)) block of isEqual().
There was a problem hiding this comment.
Added TODO to do this when documentId() is merged into master.
| @@ -24,4 +25,12 @@ describe('FieldValue', () => { | |||
| expectEqual(FieldValue.serverTimestamp(), FieldValue.serverTimestamp()); | |||
| expectNotEqual(FieldValue.delete(), FieldValue.serverTimestamp()); | |||
There was a problem hiding this comment.
Consider adding a test here as well for calling isEqual() with an object that is not an instanceof FieldValue (e.g. a string).
There was a problem hiding this comment.
Update: I reverted it since FieldPath.documentId() throws if I pass a FieldValue, and the FieldValue constructor is typed to only take FieldValues.
This PR adds the FieldValue methods to the Lite SDK.
It also solves a problem with the FieldValue methods in the current ("legacy") SDK. Our API promises that the FieldValue implementations returned by FieldValue.insertNameHere() methods are an instance of FieldValue. This was not the case anymore. I solved this by adding a wrapper type that wraps the FieldValue class in the Lite and Legacy SDK, which allows both SDKs to use different FieldValue types without duplicating all implementations.
This PR also prefixes the internal "toFieldTransform" method with an underscore since this is a class returned by the public API.