Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature/refactor #29

Merged
merged 13 commits into from
Apr 13, 2020
Merged

Conversation

lsmolic
Copy link
Contributor

@lsmolic lsmolic commented May 28, 2019

This draft PR is a suggested refactor. I have addressed some of the concerns that the community has expressed, most important being the ability to see and review ALL errors returned from the Expo API. To do this in a non-destructive, backwards compatible way, I have marked publish for deprecation and added a new method send_messages.

Here are the major changes to logic:

  1. I decided to raise an exception for all 400 and 500 level api response errors. This way we know nothing has sent and we should handle that accordingly in our code. This allows us to treat errors at the message level more individually. send_messages actually returns the ResponseHandler so we can access any number of properties on it. I believe this satisfies Return all Notification Statuses instead of just the first #21

  2. Additionally, I've added the ability to call for receipts. verify_deliveries which accepts a list of receipt_ids (This is actually a requirement listed in the docs: DeviceNotRegistered: the device cannot receive push notifications anymore and you should stop sending messages to the corresponding Expo push token. -- we should all be addressing these.

  3. Any invalid push_tokens that are flagged on the initial call are thrown into its own array so they can be handled. Not sure how often this happens, but I have actually experienced it with changing the name of an active app. Really don't recommend that, but there are cases that we would want to identify failures and pull these tokens out of rotation.

  4. I tried to catch all the edge cases for bad return values from Expo's servers. I was personally getting a lot of JSON parsing errors and it would seriously impact my apps. I believe this satisfies (fix) Validation situations where Expo returns a 200 with an error #14

  5. Added an example folder to better help newbies get up and going faster

  6. Addressed a core issue in using the gzip: true attribute. Apparently the Typhoeus webclient doesn't have you pass that as a header, it's just a regular attribute on the Request.post() call. So, this is fixed and I'm not getting 100% proper gzip responses.

  7. Added Rubocop to ensure we have similar formatting going forward.

Looking forward to your feedback.

  • Deprecated client.publish(messages) -- Backwards compatible for now
  • New send_messages and verify_deliveries methods
  • add errors, receipt_ids, and invalid_push_tokens Arrays[] to the response handler
  • Updated README
  • Bumped the version number
  • Added 2x tests to cover these additional methods
  • fixed Typhoeus implementation of gzip header (why is it a param and not a header?!?!)
  • Added RuboCop to ensure consistent formatting going forward
  • Added Examples folder
  • Added rake task for contributors to run getting started example

@lsmolic
Copy link
Contributor Author

lsmolic commented May 28, 2019

quick note that I didn't make very clear earlier: This doesn't change existing behavior. It just marks the previous methods as deprecated and throws a warn on the publish method.

@jonsgreen
Copy link

@lsmolic Thank you for your efforts here. I may not be able to look at this in the near future but hope that my client will want to support me trying it out at some point. I just wanted to at least express my gratitude in the meantime.

@lsmolic
Copy link
Contributor Author

lsmolic commented May 28, 2019 via email

lib/exponent-server-sdk.rb Outdated Show resolved Hide resolved
lib/exponent-server-sdk.rb Outdated Show resolved Hide resolved
@courtsimas
Copy link

Any progress on this?

@lsmolic
Copy link
Contributor Author

lsmolic commented Jan 14, 2020 via email

@lsmolic lsmolic marked this pull request as ready for review April 6, 2020 21:32
@lsmolic
Copy link
Contributor Author

lsmolic commented Apr 6, 2020

@audiolion @courtsimas @jonsgreen Okay fam. I addressed the concerns and then some.

I'm not sure the best option with handling >100 messages. For now, we'll throw an exception. My main concern with doing some sort of bulk processing in the library is that the responses will each have their own status, body, and errors. I don't think we'd want to obfuscate that much, and until we have a better plan, I'd like to move forward and merge this in.

Anyone want to give it a try locally?

gem 'exponent-server-sdk', git: 'https://github.com/lsmolic/expo-server-sdk-ruby.git', branch: 'feature/refactor'

@audiolion
Copy link

I unfortunately no longer am at the same company where I was working on this project to test it out, sorry 😞

@lsmolic
Copy link
Contributor Author

lsmolic commented Apr 13, 2020

@pablonahuelgomez do you have rights to review and merge?

@pablonahuelgomez
Copy link
Contributor

@lsmolic sure man, thanks for this.

Copy link
Contributor

@pablonahuelgomez pablonahuelgomez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@pablonahuelgomez pablonahuelgomez merged commit ca9dec5 into expo-community:master Apr 13, 2020
@lsmolic
Copy link
Contributor Author

lsmolic commented Apr 14, 2020

@pablonahuelgomez Thank you for the merge. What is your process for publishing the new version https://guides.rubygems.org/publishing/#publishing-to-rubygemsorg ? I'm assuming one of the maintainers does the publish.

@pablonahuelgomez
Copy link
Contributor

@lsmolic the gem has been published. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants