search

pusher / push-notifications-android

Android SDK for Pusher Beams
https://www.pusher.com/beams
MIT License
21 stars 21 forks source link

Update moshi in library and sample app and use new way of using sealed types #101

Closed daniellevass closed 4 years ago

daniellevass commented 4 years ago

What

On of our dependencies, Moshi, changed how sealed types and our polymorphic way of reading data for our server sync events.

From https://www.zacsweers.dev/a-closer-look-at-moshi-1-9/

In projects using code gen, there are cases where Kotlin classes could have (appeared to) Just Work™️ before if you forgot to annotate them with @JsonClass. These will fail at runtime now.

We needed to follow the instructions Zac's written in https://github.com/ZacSweers/moshi-sealed to make our library work work in the new system. So I've added those dependencies and followed the new ways.

Why

Users could experience a crash if they updated their own moshi dependency to 1.9.x - we shouldn't crash if they update. This should fix #100

daniellevass commented 4 years ago

Marek left some feedback that we should double check that this works okay with people who already have a queue and will be upgrading, whether the core data itself would work.

I thought we had tests that did this, and it turns out we do in PersistentJobQueueTest - we have three saved files that we access directly. However, they were failing 😱

Firstly, I've added a step in bitrise called gradle test which should run all our tests and actually notify us here if something has broken 🎉

Secondly, it looks like the edge cases where the data models have changed is now different:

Previously if we had removed an attribute from an object that attribute became "null" or we needed to provide a default. Now that object will fail to parse :(

Previously if we had added an attribute from an object, that attribute was ignored and the item was still returned. Now that object will also fail to parse :(

The current tests reflect the expected behaviour of using moshi 1.9 and have thusly been updated.

As the behaviour of this has changed I'm re-requesting reviews from you @luismfonseca - I figure as we aren't likely to change the data models in the near term this shouldn't be an issue, but should be something you're aware of!

daniellevass commented 4 years ago

I updated the tests to more clearly explain what's expected and what happens when it fails to parse a record. The source of truth should be the tests and not this PR :-)

Coincidentally, I'd noticed that the saved files didn't contain the records that I expected. So I re-ran the uncommented code to re-create the files.

marekoid commented 4 years ago

Not directly related to this PR but it would be good to see test reports in Bitrise. That is what tests have been run to be sure all of them have been discovered.