-
Notifications
You must be signed in to change notification settings - Fork 39
Allow method and body from options #18
Conversation
EventSource.js
Outdated
| } | ||
|
|
||
| xhr.send(); | ||
| xhr.send(eventsource.OPTIONS.body || undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supporting POST without supporting a body would be kind of useless, no?
Sorry my feedback wasn't that the library shouldn't also support including a body, I would just prefer that the addition of that argument be added as a second, atomic commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems odd, because then you'd end up with a spot in the commit history that doesn't make sense. i.e. I think "atomic" is a complete feature, not a single line of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "atomic" is a complete feature, not a single line of code.
For sure, I agree 100%. An atomic commit is not necessarily only a single line of code, it just happens to be in this case 😉
you'd end up with a spot in the commit history that doesn't make sense
Personally I don't see it that way. Add the ability to override xhr method seems like a fairly straight forward (and atomic) commit to add. You could make the argument that using the new functionality without being also being able to override the body doesn't make sense, but that feels colored by your described use case and isn't always the case.
But all that being said, I like to think I am flexible person so I'd be comfortable with two ways forward:
- Two commits. One only changing the
methodoption and another introducing the ability to also set the request body - One commit combining both in a fully features wrapped up as one (using the
ifelselogic in your second commit for extra clarity around the passing ofoptions.bodytoxhr.send
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version 2 is, I think, the state the PR is currently in. It turns out I can't use this method for my use case anyways - because I'm talking to a backend that doesn't "support" reconnection/resumption, and it seems this re-polls often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I squash merge we get to option 2. That's what I'll do.
I'm talking to a backend that doesn't "support" reconnection/resumption, and it seems this re-polls often.
That feature was recently added: https://github.com/jordanbyron/react-native-event-source/pull/16/files#diff-48c9e390ee9d2bf93e0587152604e3b5R125
Setting timeout to zero should disable the retry / timeout logic.
| } | ||
|
|
||
| xhr.send(eventsource.OPTIONS.body || undefined); | ||
| if (eventsource.OPTIONS.body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure how xhr.send() will handle undefined as an argument, but making the assumption that it does work, why not write this just as xhr.send(eventsource.OPTIONS.body); without the if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had done that before just in case somebody sent something falsy but not undefined. But I made it an if just to be clearer.
No description provided.