Change call to apply when invoking ApiClient

[devvmh]

I'm not sure what this will accomplish, but it sounds good. @hallettj could you explain further?

[hallettj]

Using call means that any this context in API client methods must be pre-bound. That means that a plain class cannot currently be used as an API client. Switching to the apply effect would fix that.

[devvmh]

OK I merged this, then had errors, then looked deeper into it, and can't get it. I'm not clear on what the difference between a "plain class" and what's happening now is. I'm not even sure what this context I should be passing in to apply, or why it'll help. I'll leave this open but unless someone can post a better explanation of the benefits and/or how to do it I'm leaving it out of 5.0.0 for now.

[hallettj]

Sorry for being out of communication for so long. I can look into the error
in the near future.

The issue is the usual deal where you can't just use a reference to a
method as a callback because uses of this in the method would end up
getting the value null - unless the methods are specially bound to an
object instance for each object that is instantiated.

The provided implementation of ApiClient does not have this problem because
it defines methods in the class constructor, where the new object is in
scope. But a custom API client implementation might not dynamically assign
methods at object creation time. In those cases methods must be invoked
with the context of their API client instance.

In particular, this kind of effect:

call(apiClient.fetch, ...args)

should change to this form:

apply(apiClient, apiClient.fetch, ...args)

On Thu, Aug 4, 2016, 2:44 AM Devin Howard [email protected] wrote:

Reopened #4 #4.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAllqU1gmpMQJuuZirlqTrkBBO-p_iOks5qcbSNgaJpZM4IkJRh
.

[devvmh]

Which in the current redux-saga can be written as

call(apiClient, apiClient.fetch, ...args)

if I'm not mistaken.

Is this a breaking change? Either way, I'll make a pull request and leave it for you to review. Last time the saga tests broke so it may be a few tries to get it merged in.