Refactor MapEvents listeners, each listener will include an event data property.#718
Refactor MapEvents listeners, each listener will include an event data property.#718
Conversation
|
overall LGTM but let's first fix all CI issues |
pengdev
force-pushed
the
peng-refactor-map-listeners
branch
from
d516c57 to
232f02d
Compare
pengdev
force-pushed
the
peng-refactor-map-listeners
branch
from
232f02d to
ac8c4a7
Compare
pengdev
force-pushed
the
peng-refactor-map-listeners
branch
from
cdb09b6 to
5cc1434
Compare
…ode to the reason enum.
pengdev
changed the title
| import com.mapbox.maps.extension.observable.eventdata.ResourceEventData | ||
|
|
||
| private val gson by lazy { | ||
| Gson() |
There was a problem hiding this comment.
however could it technically happen that user is parsing several instances of event data from multiple threads? guess it's too advanced though 😄 so let's keep as is
There was a problem hiding this comment.
According to Gson 's api doc
Gson is typically used by first constructing a Gson instance and then invoking toJson(Object) or fromJson(String, Class) methods on it. Gson instances are Thread-safe so you can reuse them freely across multiple threads.
There was a problem hiding this comment.
how about koltin serialization?
maybe not in this PR
There was a problem hiding this comment.
how about koltin serialization? maybe not in this PR
we were using gson library to serialise/deserialise in our codebase, it would be good to evaluate and replace it across the codebase in a separate PR.
There was a problem hiding this comment.
the gson part mostly stems from our "java" days. Kotlin serialisation would be a good thing to evaluate, additionally a geojson rewrite in kotlin ;)
| fun Event.getResourceEventData(): ResourceEventData { | ||
| val json = data.toJson() | ||
| return Gson().fromJson(json, ResourceEventData::class.java) | ||
| return gson.fromJson(json, ResourceEventData::class.java) |
There was a problem hiding this comment.
should we add try-catch block?
There was a problem hiding this comment.
imho if the data format has been changed, it should be a bug and should throw exceptions so that we can fix it in a patch.
There was a problem hiding this comment.
yeah, but is it proper behavior for users? is it possible that our users (developers) can release some app in play market and got crash in app?
There was a problem hiding this comment.
do you suggest we return nullable EventData instead? If it's a bug inside our code, we could provide non-breaking patch releases to fix it. And so far this has been the default behaviour and no bug report on it yet 🙂
There was a problem hiding this comment.
we can discuss it with the team. probably leave as is for now. but let's think about it
several options for now:
- return nullable
- support some default value and work with it somehow
also we can send telemetry event for such crashes
There was a problem hiding this comment.
Sounds good, let's make it as a follow up 👍
There was a problem hiding this comment.
yeah, but is it proper behavior for users? is it possible that our users (developers) can release some app in play market and got crash in app?
That statement could be true for every line of code we ship. The program is expected to return this non null data in that specific format, it's part of the contract that gl-native makes with us. Overall, I'm personally leaning more towards a crash-fast approach, we catch issues during development, else they can become unnoticed and broken if not properly exercised. Additionally requiring developers to handle null adds logical branching to their code. This makes it more complex to handle, while it's not expected program output.
| import com.mapbox.maps.extension.observable.eventdata.ResourceEventData | ||
|
|
||
| private val gson by lazy { | ||
| Gson() |
There was a problem hiding this comment.
how about koltin serialization?
maybe not in this PR
| * @return a parsed MapLoadedEventData object. | ||
| */ | ||
| fun Event.getMapLoadedEventData(): MapLoadedEventData { | ||
| val json = data.toJson() |
There was a problem hiding this comment.
@pengdev, in a following releases, if native team would provide a formal spec for events, we could auto-generate MapLoadedEventData and remove gson conversion, right? Modification of an internal logic would be a non-breaking change?
There was a problem hiding this comment.
We could generate the MapLoadedEventData using the formal spec, but I think we still need to deserialise it using gson or koltin serialization
Modification of an internal logic would be a non-breaking change?
Addition of properties inside MapLoadedEventData shouldn't be a breaking change, however, we should not remove or modify the existing properties.
6 participants
PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >
Pull request checklist:
@JvmOverloads,@file:JvmName, etc).mapbox-maps-androidchangelog:<changelog>Refactor MapEvents listeners, so that each listener will include one event data property.</changelog>.Summary of changes
This PR refactors the MapEventListeners, so that each listener will include one event data property. This unblocks us to add properties to the event data incrementally without breaking our public API.
List of affected listeners:
This PR also refactors the package name for Map event's data models to
com.mapbox.maps.extension.observableinside thesdk-basemodule, so it can be reused across different plugins.And fixed the type of TileID's z/x/y properties.
User impact (optional)