gitbeaker
gitbeaker copied to clipboard
jdalrymple
requesterFn is hard to write because of reliance on secret "options handler"
Description
Hi jdalrymple! It's been a while. My team is finally moving onto your wonderful package and only ran into one hiccup. Sorry if this is a Feature request; I'm classifying this as a Bug because it seems more like an oversight in implementing the exposure of requesterFn.
We were surprised that we had to set our own content-type on JSON requests, and even more surprised that some arguments (such as source/target branches on creating merge requests) seemed to be filling the request body with camelCase versions of the parameter names.
The issue appears to be that the default request functions make use of sanitizing code that, in the absence of contrary documentation, a programmer implementing requesterFn would reasonably assume had already been taken care of by the library. Indeed, since improper body params are based on internal local variable names in the library, requestFn implementers need to peer deep into the source code just to confirm the exact way in which body parameters will need to be fixed.
Steps to reproduce Implement a requesterFn assuming that the options provided by the library are appropriate for most use cases.
Expected behaviour The library has fully prepared the headers and body before passing it to the custom requesterFn so that making a request in the most expedient way possible will probably work in most cases.
Actual behaviour Object representation of request body is not fully transformed and prepared for transmission, among other issues with headers and possibly form data.
Possible fixes
- Pre-run the defaultOptionsHandler before passing the results to the appropriate requesterFn, or
- Alternatively, provide detailed documentation about how to implement requesterFn which conspiculously mentions these caveats.
Yay!! glad to hear you guys making the move.
Thats a fair enough request! To be completely honest, it was definitely overlooked. The whole point of the requesterFn is such that you shouldn't HAVE to look deeper into the code, plus with the lack of docs, its just an unpleasant experience.
I know I can make it much better, just have to spend a little time looking over what I have going on there. Currently been focused on finishing #1104 which I'm almost on the finished line. What is you're timeline on this fix up? I'm guessing its a blocker, but is it a ASAP or could it wait a week or so ?
For now we've imported xcase into the project and basically copy-pasted your implementation of the default optionsHandler, so this isn't blocking us at all. It just slowed us down a bit trying to figure out why requests were malformed, and I wanted to save future developers the headache.
Since this is extremely related: I'm currently trying to obtain job artifact files. The relevant API documentation suggests that the response will always be streaming.
Setting the documented option {stream: true} results in the underlying got requester's request method being set to STREAM, which is incorrect. The root cause here is that the requesterFn incorrectly enforces the HTTP method to be equal to the RequesterType method invoked. For requester.stream that's very likely to be incorrect.
This seems to be another caveat to add to the documentation :)
@Vogel612 Yea you're right. I really need to review all of this stuff, just been slow recently. Who would of thought adding types for everything would take so long :joy: