Fix the Server Timestamp README example, which does not compile - #849
Open
anggrayudi wants to merge 1 commit into
Open
Fix the Server Timestamp README example, which does not compile#849anggrayudi wants to merge 1 commit into
anggrayudi wants to merge 1 commit into
Conversation
The Server Timestamp snippet could not compile, in five separate ways: - `val timestamp: Timestamp = Timestamp.ServerTimestamp` is a type error. `ServerTimestamp` is declared as `data object ServerTimestamp : BaseTimestamp`, so the field has to be typed `BaseTimestamp`. This is what people actually hit when following the docs. - `timestamp` was declared twice as a constructor parameter of the same class. - `val timestamp = ServerValue.TIMESTAMP` and `val alternativeTimestamp = FieldValue.serverTimestamp` omit the type, which constructor parameters may not do. - a trailing comma after `@Serializable(with = DoubleAsTimestampSerializer::class)` is a syntax error. - `DoubleAsTimestampSerializer.serverTimestamp` does not exist; the constant is `DoubleAsTimestampSerializer.SERVER_TIMESTAMP`. Split the Realtime Database and Cloud Firestore cases into separate snippets, since the original combined them into one class with a duplicated field name, and note why the Firestore field is a `BaseTimestamp`. Fixes GitLiveApp#666
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #666.
Problem
The Server Timestamp example in the README cannot compile. @marcelpallares hit the first item below, and @mattegoghero followed up with "can't use the ServerTimestamp as written in the documentation" — the snippet is broken in five independent ways:
val timestamp: Timestamp = Timestamp.ServerTimestampis a type error.ServerTimestampis declared asdata object ServerTimestamp : BaseTimestamp— it is not aTimestamp. The field has to be typedBaseTimestamp. This is the exact thing reported in the issue.timestampis declared twice as a constructor parameter of the same class.val timestamp = ...andval alternativeTimestamp = ...omit their types, which constructor parameters may not do.@Serializable(with = DoubleAsTimestampSerializer::class)is a syntax error.DoubleAsTimestampSerializer.serverTimestampdoes not exist. The constant isDoubleAsTimestampSerializer.SERVER_TIMESTAMP.Fix
Split the Realtime Database and Cloud Firestore cases into separate snippets — the original folded both into one class, which is where the duplicated field name came from — and correct the types and the constant name. I also added a short note on why the Firestore field is a
BaseTimestamp, since that is the part that surprises people.Every symbol in the new snippets is checked against the source rather than assumed:
ServerValue.TIMESTAMP: ServerValue, andServerValueis@Serializable(with = ServerValueSerializer::class)Timestamp.ServerTimestamp : BaseTimestamp, andBaseTimestampis@Serializable(with = BaseTimestampSerializer::class)FieldValue.serverTimestamp: FieldValue, andFieldValueis@Serializable(with = FieldValueSerializer::class)DoubleAsTimestampSerializer.SERVER_TIMESTAMP: DoubleScope — the second half of the issue is not addressed here
The issue also reports a runtime failure when writing a
ServerTimestamp:I deliberately did not touch that, because I could not reproduce it and the encoding path looks correct:
BaseTimestampSerializer.toNativeValuemapsTimestamp.ServerTimestamptoFieldValue.serverTimestamp.nativeValueTimestampTests.serializers()already asserts that(Timestamp.ServerTimestamp as BaseTimestamp).firebaseSerializer()resolves toBaseTimestampSerializer, and it has done so since April 2023 — before this issue was filedDocumentReferenceTest.testServerTimestampFieldValuewrites aServerTimestampend to endsetnow takes an already-encodedEncodedObject, so the native SDK never sees an unencoded Kotlin valueThe stack trace shows Google's
CustomClassMapperreflecting over the GitLive object, i.e. it arrived at the Android SDK unencoded, which is what @Daeda88 suspected. That points at either an older version or a call path that bypasses the GitLive encoder. Since @marcelpallares never came back on @Daeda88's questions, there is nothing further to go on — so this PR fixes only the documentation defect, which is real, verifiable, and is what both reporters actually ran into.