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.
boy protobuf and typescript are tricky beasts...
So there were a few edge-cases around the way we type, encode and decode observation position metadata.
We were using
fromPartial
for encoding metadata, which assigns defaults for missing props, which meant that decoding would pass type checks, but defaults would be assigned to the missing props, in this caselongitude
andlatitude
, which is a "null island" bug.This PR makes a few changes:
manualLocation
is now optional in protobuf - the distinction between when this isundefined
(when there is no location for the observation) and when it is boolean (when there is a location) is important.gpsAvailable
,networkAvailable
,passiveAvailable
props ofmetadata.positionProvider
are now optional on the protobuf, to distinguish between cases when this data is not available on the client (iOS) and when it is.locationServicesEnabled
ofmetadata.positionProvider
is now required on the JSONSchema - if we do store position provider metadata, then we have this data - this corresponds to the Expo types.timestamp
andcoords
are now required fields ofposition
andlastSavedPosition
in the JSON schema. These metadata options don't make sense without this data.timestamp
orcoords
(possible, because of the way protobuf make everything optional), then we strip the position prop from metadata (rather than the alternative of assigning defaults or throwing)This PR also removes usage of
fromPartial
, and stops generating it since it's no longer needed. Using this is "dangerous" because it assigns default values deeply to a message, which can lead to unexpected results - data that is type safe, but might have required fields set to default values.Impact on frontend:
This is breaking, because it changes types to make some fields required. However fields that are newly set to required are also required / always set on the Expo Location
LocationObject
andLocationProviderStatus
. Therefore in frontend code this should not break things, because these values are being set from these objects from Expo.These changes are not really specific to Expo - it would not make sense to add this metadata without these required fields being available, even if we move away from Expo for reading location.
Why is this important?
In the future, this (position) metadata is potentially important for "proofs" of observations. Without these changes it is possible that default values could be unexpectedly set, e.g. values could be
false
when they were actually unknown (undefined
).