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

Adds X-Parse-Push-Status-Id header #1412

Merged
merged 3 commits into from
Apr 7, 2016
Merged

Adds X-Parse-Push-Status-Id header #1412

merged 3 commits into from
Apr 7, 2016

Conversation

flovilmart
Copy link
Contributor

fixes #1157

@@ -38,7 +38,7 @@ export class PushController extends AdaptableController {
return !!this.adapter;
}

sendPush(body = {}, where = {}, config, auth, wait) {
sendPush(body = {}, where = {}, config, auth, pushStatusIdCallback = () => {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that's not super nice, but I didn't want to break the returned promise that allow us to test the push flow.

Open to suggestions

@codecov-io
Copy link

Current coverage is 92.77%

Merging #1412 into master will increase coverage by +0.03% as of e037011

@@            master   #1412   diff @@
======================================
  Files           86      86       
  Stmts         5433    5442     +9
  Branches      1004    1005     +1
  Methods          0       0       
======================================
+ Hit           5039    5049    +10
  Partial         10      10       
+ Missed         384     383     -1

Review entire Coverage Diff as of e037011

Powered by Codecov. Updated on successful CI builds.

@drew-gross
Copy link
Contributor

This seems reasonable given the constraints, but shouldn't the POST handles be waiting to respond until after the sendPush function is done anyway?

@flovilmart
Copy link
Contributor Author

That can be long, and defeats the purpose of the push status... Open to change it though but that will impact negatively the performance, I don't believe parse.com was waiting for it to be done.

@drew-gross
Copy link
Contributor

Hmmm I'm not super familiar with the model we are using here, it must be different from Parse.com. In Parse.com we write a push status object, then read from it to actually send the push. We respond to the request once the push status object is written, not when the push has finished. Is that not how it works here?

@flovilmart
Copy link
Contributor Author

we don't wait for anything in that case. I could wait for the initial _PushStatus to be stored before calling back.

@drew-gross
Copy link
Contributor

I think this makes more sense. It's too bad we rely on the result of the long-running promise, or we could return the earlier promise. Can you call your callback function onPushStatusSaved or something similar instead? Other than that this looks good now.

@flovilmart
Copy link
Contributor Author

waiting for travis then merging

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart merged commit bc96f0b into master Apr 7, 2016
@flovilmart flovilmart deleted the push-status-header branch April 7, 2016 22:08
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.

Return PushStatus ID from push endpoint.
4 participants