feat: add support for custom request types #190
Conversation
lib/request.js
Outdated
| @@ -121,6 +122,10 @@ function Request (options) { | |||
| simulate: options.simulate || {} | |||
| } | |||
|
|
|||
| if (options.customRequestType) { | |||
| util.inherits(this.constructor, options.customRequestType) | |||
There was a problem hiding this comment.
This alters the constructor, which is a singleton property. This means that after the first execution, all the constructors will have this set. Moreover concurrent usage will be broken too.
Could we move this block next to the other util.inherits?
There was a problem hiding this comment.
@mcollina you're right about the constructor. I've updated the code so that custom requests are dealt independently from Request.
About moving this util.inherits call down next to the other one: that'd remove the proposed generality, ie users wouldn't be able to provide specific classes/functions from which the request would inherit. Or am I missing something?
In any case this is more of a curiosity than anything. With the new approach this shouldn't be needed.
lib/request.js
Outdated
| if (options.customRequestType) { | ||
| assert(options.customRequestType.prototype !== undefined) | ||
| return new MakeCustomRequest(this, options) | ||
| } |
There was a problem hiding this comment.
Can you move this if to where Request is instantiated?
There was a problem hiding this comment.
My idea was to keep it transparent to the lib/request clients. But sure, will do
|
good work! |
|
CI is failing |
Co-authored-by: KaKa <climba03003@gmail.com>
I'd be ok with either keeping it as is or updating. I'm usually not great at naming things haha. Instead of One more thing: did you use pascal case intentionally? Given the other options aren't capitalized wouldn't that be a, yet small, inconsistency in the lib's public api? |
Since it can be a I think it is acceptable for the option to be |
This PR adds support for a new
optionparameter:customRequestType. It represents a custom type from which therequestobject should inherit besidesstream.Readable.Closes #189.
Checklist
npm run testandnpm run benchmarkand the Code of conduct