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

Push notifications to APNS per batches #10

Merged
merged 2 commits into from
Jul 8, 2016
Merged

Conversation

flovilmart
Copy link
Contributor

Fixes #9

@codecov-io
Copy link

Current coverage is 87.50%

Merging #10 into master will increase coverage by +0.48% as of f6a1ce1

@@            master    #10   diff @@
=====================================
  Files            5      5       
  Stmts          239    248     +9
  Branches        38     40     +2
  Methods          0      0       
=====================================
+ Hit            208    217     +9
  Partial          1      1       
  Missed          30     30       

Review entire Coverage Diff as of f6a1ce1

Powered by Codecov. Updated on successful CI builds.

@codecov-io
Copy link

codecov-io commented Mar 31, 2016

Current coverage is 88.51%

Merging #10 into master will increase coverage by 0.39%

@@             master        #10   diff @@
==========================================
  Files             5          5          
  Lines           261        270     +9   
  Methods          28         30     +2   
  Messages          0          0          
  Branches         45         46     +1   
==========================================
+ Hits            230        239     +9   
  Misses           31         31          
  Partials          0          0          

Powered by Codecov. Last updated by d789ef6...7c0235d

@flovilmart
Copy link
Contributor Author

@drew-gross let me know what you think as it really changes the game


let promises = devices.map((device) => {
// Start by clustering the devices per connections
let devicesPerConnIndex = devices.reduce((memo, device) => {

Choose a reason for hiding this comment

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

I'll be honest, I don't really like this usage of reduce. reduce is from a functional paradigm, but when you modify memo inside your reduce, that feels very imperative. I'd rather see imperative code use imperative paradigms. If you wanted to copy memo with a modification, that would feel better, I think. Or just use a for loop. Or go back to map, and have the mapped function return a (connIndex, device) pair, since you just end up iterating over the (connIndex, device) pairs in arbitrary order anyway.

@drew-gross
Copy link

Seems fine to me but I'm not really the best person to review push related code. @bnham or @nlutsenko would probably be better.

@mihai-iorga
Copy link

this pull request it's somewhat forgotten :)

@fasa23
Copy link

fasa23 commented May 17, 2016

@Darkkiss thats why I ended up implementing OneSignal push adapter, not a good option tough since I use cloud code to push to thousands of installations and I don't know if parse server takes just to many seconds determining which installations should receive the push notification, so it still takes like 3-4min to push to 20k installations which is not a good option since I want to notiffy soccer matches goals and right now all of my "competence" apps are notifying faster than mine and I'm losing lots of users. If I push directly from OneSignal, the notification arrives in just a few seconds but I don't know how to tell OneSignal that I updated a record in my database other than using cloud code and the push adapter. If you find a better alternative please let me know

@mihai-iorga
Copy link

@flovilmart it's there any change for some hints about this adapter ... I would like to do it .. but don't know exactly where to start :( .. my applications have more than 1.000.000 devices with push notifications and I cannot send a push to all in seconds.

@flovilmart
Copy link
Contributor Author

@mihai-iorga I believe the scheduling/batching of the push could be better handled. but that would require some changed on parse-server in order for the adapter to run the query, slice it in batches and schedule it properly

@flovilmart flovilmart force-pushed the improveAPNSBatching branch from 3e96048 to 7c0235d Compare July 7, 2016 19:05
@flovilmart flovilmart merged commit a9089ec into master Jul 8, 2016
@flovilmart flovilmart deleted the improveAPNSBatching branch July 8, 2016 12:46
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