GitLiveApp
Firestore customizable encoding for where clauses and update methods
Fixes #604 and trumps #605
This solution extends all update methods in Firestore to support more advanced update methods. String fields and FieldPaths can now be used interchangably and custom serializer can be passed.
documentRef.update("field" to "value, "otherField" to 1)
becomes
documentRef.update {
// specifying build settings is optional
buildSettings = {}
"field" to "value"
FieldPath("otherField") to 1
"customField".to(CustomValueSerializer(), customValue)
}
Note that the existing vararg pairs can still be used.
Similar support has been added to where clauses as well as startAt/endAt/etc:
query.startAt {
add(value)
addWithStrategy(CustomValueSerializer(), customValue)
}
There is a small regression on the existing update methods in that
public fun update(documentRef: DocumentReference, vararg fieldsAndValues: Pair<String, Any?>, buildSettings: EncodeSettings.Builder.() -> Unit)changed to public fun update(documentRef: DocumentReference, buildSettings: EncodeSettings.Builder.() -> Unit, vararg fieldsAndValues: Pair<String, Any?>) due to Kotlin not being able to figure out which update method to use.
Furthermore, I've split the Firestore tests into a few different files and extended them so they clean up all data after completion.
Lastly, I discovered bug #613 and fixed it
I still need to add Readme/some tests, but feedback would already be appreciated @nbransby as I wont be able to continue until next week
Now fully ready for review @nbransby / @Reedyuk . See description above for details of changes.
There is a small regression on the existing update methods in that
Does that make this a breaking change?
I still need to add Readme/some tests, but feedback would already be appreciated @nbransby as I wont be able to continue until next week
looks great to me, I would like to see more comprehensive documentation in the README that would also help me understand the intended usage.
There is a small regression on the existing update methods in that
Does that make this a breaking change?
Yeah, its breaking. I went from:
update(docRef, "path" to value) { encodeDefaults = false }
to
update(docRef, { encodeDefaults = false }, "path" to value)
The reason I had to do something is because the new update method has the signature:
update(docRef) {
"path" to value
}
and since the original method takes a vararg, it can be called as update(docRef) { encodeDefaults = false }, which means the compiler wont know wich method to call. I considered alternatives:
- Change the original code to always take at least one path (i.e.
fun update(docRef, firstValue: Pair<String, Any>, vararg others: Pair<String, Any>)but that would mean you woundnt be able to call it withupdate(docRef, *listOfPairs.toTypedArray())which I assume is a common usecase. - Name the new update function something else, e.g.
updateWith, but I though the downside of that was that it sort of breaks the agreement that we name stuff like it is named in the native libraries - I do this approach, knowing that the
encodeSettingsclosure is relatively new and will thus probably be not used that much yet. - Keep the old signature and the new one as is now, but that means you have to use named arguments for the closure which sucks.
I think 1 is a definitive no-go. If you prefer we do 2, Im fine with that as well. Its always gonna be a tradeoff so its pick your poison basically.
As for documentation, Ill change that as soon as we agree on this point :)
I spent long enough trying to think of additional alternatives without joy so, I favour 2 and calling it updateFields
does startAtFieldValues/startAfterFieldValues/endAtFieldValues/endBeforeFieldValues suffer the same signature clash issue as update or can they renamed to startAt/startAfter/endAt/endBefore` overrides?
@nbransby @Reedyuk Sorry for the looooooong delay in picking this up. Priorities just got in the way so I never got around to addressing this.
I've tracked latest master, corrected the readme and where possible moved things out of the public API.
On startAtFieldValues etc, yes, unfortunately
public fun startAt(builder: FieldValuesDSL.() -> Unit)
would clash with the existing
public fun startAt(vararg fieldValues: Any?, buildSettings: EncodeSettings.Builder.() -> Unit)
as kotlin would not know which closure to use if you just call startAt {}, assuming fieldValues to be empty. The alternative is of course to replace the latter with public fun startAt(firstFieldValue: Any?, vararg fieldValues: Any?, buildSettings: EncodeSettings.Builder.() -> Unit), as calling startAt without any fieldValues in the first place is probably wrong, but since that would break the API Im guessing nobody is in favor of that.
On some of the things in the API: see individual comments.
@Daeda88 is this still a thing?
If so, it would be really nice if you rebase and create a PR here: https://github.com/DatL4g/firebase-kotlin-sdk
@Daeda88 looks good, lets deprecate the old methods (update, startAt, endAt) with the plan to rename these new ones eventually in a future 3.x release then I will merge
@nbransby added deprecation/updateWith annotation. Hope its finally good to go! (almost exactly a year further but worth it 😂 )
Glad to see its approved. Im assuming you're holding of on merging due to the CI issue, but I have no idea what's causing it as API dump doesnt trigger any changes for me locally
Seems to be that there is changes it wants to push but doesn't have the permission to push. The intention was to get it to autocommit the changes but i think we are going to have to just make it fail instead. The issue is because of permissions and forks, i guess its because the action runs on gitlives workspace and so doesnt have write access to the forked branch.
Im not sure if @Daeda88 is on a mac, but it could be that we generate the ios api too now, whereas previously it was only done on jvm and js
@Reedyuk API is only for JVM targets (android/jvm) and I am running on Mac regardless. So that cant be it. Running it now with a git status command to see what its trying to commit. Also, what's the point of the auto commit task if it doesnt work?