refactor: remove configurable request #394
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #394 +/- ##
==========================================
- Coverage 98.52% 98.51% -0.01%
==========================================
Files 5 5
Lines 338 336 -2
Branches 51 50 -1
==========================================
- Hits 333 331 -2
Misses 3 3
Partials 2 2
Continue to review full report at Codecov.
|
2 similar comments
Codecov Report
@@ Coverage Diff @@
## master #394 +/- ##
==========================================
- Coverage 98.52% 98.51% -0.01%
==========================================
Files 5 5
Lines 338 336 -2
Branches 51 50 -1
==========================================
- Hits 333 331 -2
Misses 3 3
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #394 +/- ##
==========================================
- Coverage 98.52% 98.51% -0.01%
==========================================
Files 5 5
Lines 338 336 -2
Branches 51 50 -1
==========================================
- Hits 333 331 -2
Misses 3 3
Partials 2 2
Continue to review full report at Codecov.
|
| @@ -178,8 +171,6 @@ class ServiceObject<T = any> extends EventEmitter { | |||
| this.methods = config.methods || {}; | |||
| this.interceptors = []; | |||
| this.Promise = this.parent ? this.parent.Promise : undefined; | |||
| this.requestModule = | |||
There was a problem hiding this comment.
It may be nice to throw if config.requestModule is set.
There was a problem hiding this comment.
I seem to remember @stephenplusplus smacking my hands last time I did something like that in this exact spot :)
There was a problem hiding this comment.
Haha. Well, throwing developer errors hasn't been something we have done before. We only throw errors that make sense to the user, and leave it to our various testing mechanisms to cover how we use our own libraries.
There was a problem hiding this comment.
This is a different use-case though. We are detecting that the user didn't read the changelog and is passing a config flag that we are going to silently ignore. It would be nice to tell them that there was a breaking change. I don't have objections to the general disposition about throwing errors in these libraries; but this is different.
There was a problem hiding this comment.
The users are us, though.
Sorry, submitted prematurely. But, the difference I see is that this library is just an abstraction of formerly internal code to the various APIs. So, it's not that users are providing this option, it was only done by us from the constructors in those APIs.
There was a problem hiding this comment.
I am wildly irresponsible and really rely on TypeScript types and aggressive exceptions to save me from myself 🤷♂️
There was a problem hiding this comment.
I've seen (and like) the pattern of asserting that arguments/config are correct at the start of a function, so that a client fails as early as possible when interacting with the library (vs., having undefined behavior that bubbles as an error like can't access property of undefined).
Having said this, given we're calling this a breaking change and, like @stephenplusplus says, this is an interface probably mainly used by ourselves, I'd argue against the throw (which we'll then wonder about why the heck it's in the codebase for the next 5 years).
BREAKING CHANGE: This PR removes the ability to configure a custom implementation of the
Requestmodule. This was necessary when we were migrating fromrequesttoteeny-request, but that migration is now complete. All interfaces at accepted a custom implementation ofrequestwill no longer accept one.teeny-requestis now just included in the box.This change is being made to simplify the
requestinterface touch points.