Move IRequestExecutor to APIContext

[kilink]

APIRequest delegates to IRequestExecutor to make all HTTP calls; currently a default implementation is used and stored in a static field in APIRequest, with static methods which can be used to override it.

This makes it awkward for testing, as you then have global state you need to setup and teardown, which also precludes parallelizing test runs. It also makes it impossible to use different IRequestExecutors for different kinds of requests without synchronization or some sort of coordination (admittedly I don't have a use case for this).

Wouldn't it make more sense for the IRequestExecutor configuration to live somewhere else, for example APIContext?

[JiamingFB]

Hi kilink,

Thanks for the suggestion. I agree that it makes more sense to let the http request configuration flow with APIContext, so we may have multiple configs for different users. We'll think about adding the config into APIContext to make it more flexible, while still keeping the current global 'default' config, which is easier to code and covers most of the use cases.

[kilink]

I would argue that the current global configuration is not easier to code with. As I mentioned, it makes testing more awkward and limited, and it means that my code that my Facebook related code can never really be isolated, since it must set some global state.

Imagine I wanted to call two sets of APIs with different retry policies. Maybe for certain calls, I want to retry several times, but for others it's okay if it fails immediately. With the current approach, I would need to put all of that logic in the request executor itself, since I am not able to set different IRequestExecutor instances for use with different sets of APIs.

I don't think we lose any usability by moving the IRequestExecutor configuration to the context object. All that needs to be done to maintain the current behavior is to just initialize the context with the current default request executor. That will keep the current behavior, and allow consumers to customize and test things more easily.

I can open a PR for this if you think this approach makes sense.

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. Thank you for your contributions.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.