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

Remove Client class in favor of direct axios instance #6

Open
jjnesbitt opened this issue Mar 19, 2020 · 5 comments
Open

Remove Client class in favor of direct axios instance #6

jjnesbitt opened this issue Mar 19, 2020 · 5 comments
Assignees

Comments

@jjnesbitt
Copy link
Member

This has been a source of pain for me several times in the past. I don't see a real reason to wrap calls with the Client class within multinetjs itself. The result of having this abstraction is less flexibility (as you can't just pass any axios parameters into the exposed API methods), which results in both more changes/releases to multinetjs, as well as workarounds.

I think we should remove the Client field in the MultinetAPI class altogether, and replace it with an axios instance. Along with this, we should have a parameter in each function that accepts an AxiosRequestConfig object, which we could just pass to the axios call, allowing more flexibility and less multinetjs releases.

The other benefit of replacing these wrapping calls with axios calls directly, is that the client can have access to the details of the request. This can be useful for several reasons, and has already been required in the project. This would result in a breaking change to multinetjs however, so if we were to move forward with this, we'd have to coordinate with that repository as well.

I believe I've already discussed this somewhat with @waxlamp, but I've again run into roadblocks as a result of this, so that's why I'm raising this issue now. I'm willing to do much of the work required for this, but I want to make sure we're in agreement first. Please let me know what you think.

@waxlamp
Copy link
Contributor

waxlamp commented Mar 20, 2020

Let's go ahead and try this. I suspect you are right about everything you've said. I'm a bit uncomfortable with having Axios be part of the interface of multinetjs, but my metabrain says I'm uncomfortable about nothing.

When you do this change, try to keep it minimal to just changing out the client stuff, and please open parallel PRs in the client and the view repositories as well (of course, @JackWilb and I can help with those).

Thanks, @AlmightyYakob, good thinking on this.

@jjnesbitt
Copy link
Member Author

jjnesbitt commented Mar 20, 2020

So for the initial change, should I preserve functionality on multinet-client and others? As in, keep the return types, just change the internals?

@waxlamp
Copy link
Contributor

waxlamp commented Mar 20, 2020

Yes I believe so. Using axios instead of the Client class shouldn't change the library interface right?

@jjnesbitt
Copy link
Member Author

Yes I believe so. Using axios instead of the Client class shouldn't change the library interface right?

No it shouldn't, but there's really two issues I proposed, one being using the client class, the other being the extraction of the return values. I'm not sure how you feel about the second point, but I can start the first one without issue.

@waxlamp
Copy link
Contributor

waxlamp commented Mar 20, 2020

Ah gotcha. Yeah we will need to have a deeper discussion about that second point because the intent of multinetjs is to provide a simple interface to the API. I'll have to hear about why the request transport mechanism needs to be exposed, etc., before we decide on that breaking change.

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

No branches or pull requests

2 participants