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

Move axios request config option into main uploadTable options argument #16

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

waxlamp
Copy link
Contributor

@waxlamp waxlamp commented Aug 3, 2020

This cleans up the calling sequence for uploadTable, enabling the caller to place any axios config values into a special entry in the options argument, rather than having to supply it as a separate argument. This makes the calling sequence easier to parse in the code, and makes it explicit what the formerly separate argument is for.

The two calls to uploadTable in the multinet-client codebase will benefit from this change.

@waxlamp waxlamp requested a review from jjnesbitt August 3, 2020 18:31
@jjnesbitt
Copy link
Member

I'm actually opposed to this change, mainly because I think all (or at least most) of the methods in the API should have this option associated with it, and since they don't all contain an options parameter, I think having a final optional config parameter is the most consistent way to provide this option to all of the methods. It may lead to a few messier function calls, but those calls aren't exactly pretty to begin with anyway.

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 3, 2020

I'm actually opposed to this change, mainly because I think all (or at least most) of the methods in the API should have this option associated with it, and since they don't all contain an options parameter, I think having a final optional config parameter is the most consistent way to provide this option to all of the methods. It may lead to a few messier function calls, but those calls aren't exactly pretty to begin with anyway.

The thing is, these are in fact options for the function call. If we have to add an axios request config to each function's specialized options, that's not really an issue for me. And the fact that the function calls are not pretty to begin with is not a good argument for making them even uglier 🐱

We can also leverage typescript for this if needed: there can be a general Options type that contains the axios request config, and the individual option types for each function can simply extend it.

I remain strongly in favor of this, because eventually people besides us will be using this library, and having two separate option arguments for each function, the latter of which is optional, will be very confusing. Both pain points are solved simply by allowing people to specify their axios options in the same place as all other options.

@jjnesbitt
Copy link
Member

@waxlamp and I discussed more offline, and decided that including the axiosRequestConfig parameter in a more generally purposed Options type would be the way to go. For exposing this config to other functions, it's a matter of either adding an options: Options parameter, or changing the type of the existing options parameter to extend the new Options type.

Copy link
Member

@jjnesbitt jjnesbitt left a comment

Choose a reason for hiding this comment

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

Regarding my previous comment, my ask would be to factor out the axiosRequestConfig field into the general Options interface, and changing FileUploadOptionsSpec to extend that.

@waxlamp
Copy link
Contributor Author

waxlamp commented Aug 5, 2020

Regarding my previous comment, my ask would be to factor out the axiosRequestConfig field into the general Options interface, and changing FileUploadOptionsSpec to extend that.

Done in d27d043.

We're set up to add axiosRequestConfig to the other functions, though I don't think we should do it until it's needed (or at least, if we do it without a specific need, let's do it on a different PR).

@waxlamp waxlamp requested a review from jjnesbitt August 5, 2020 17:07
@jjnesbitt
Copy link
Member

Done in d27d043.

We're set up to add axiosRequestConfig to the other functions, though I don't think we should do it until it's needed (or at least, if we do it without a specific need, let's do it on a different PR).

Agreed

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.

None yet

2 participants