feat: add new hooks api #3636
feat: add new hooks api #3636
Conversation
ronag
force-pushed
the
newhooks2
branch
6 times, most recently
from
1fbf4d4
to
f13993b
Compare
ronag
requested review from
Uzlopak,
metcoder95 and
mcollina
and removed request for
Uzlopak,
metcoder95 and
mcollina
There was a problem hiding this comment.
My understanding is that this is fully backward compatible?
I think we should bump the version on the global dispatcher symbol:
Line 5 in 90e2e13
And then add an adapter to the "1" API, so that fetch keeps working.
Also... tests are missing.
| @@ -32,7 +127,7 @@ module.exports = class DecoratorHandler { | |||
|
|
|||
| onResponseStarted (...args) { | |||
| assert(!this.#onCompleteCalled) | |||
| assert(!this.#onErrorCalled) | |||
| // assert(!this.#onErrorCalled) | |||
There was a problem hiding this comment.
We have a bug in the h2 client that wasn't caught before.
There was a problem hiding this comment.
@metcoder95: client-h2. calls onResponseStarted after onError
There was a problem hiding this comment.
Hmm, interesting; I imagine this happens if something failed before dispatching the request, or how do you find it?
There was a problem hiding this comment.
I applied the assertions to the core request handling:
Yes
Any specific reason why you think we should bump it?
👓 |
ronag
force-pushed
the
newhooks2
branch
5 times, most recently
from
86251ec
to
4736a1b
Compare
|
@Uzlopak do you think you could help with tests? |
|
This needs more work but I think it's mostly semver minor. |
| let ret = true | ||
|
|
||
| if (this.#handler.onResponseData?.(data) === false) { | ||
| ret = false |
| if (this.#handler.onResponseHeaders?.(headers, statusCode) === false) { | ||
| ret = false | ||
| } | ||
|
|
||
| if (this.#handler.onHeaders) { | ||
| const rawHeaders = toRawHeaders(headers) | ||
| if (this.#handler.onHeaders(statusCode, rawHeaders, this.#resume) === false) { | ||
| ret = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Here we will have consistency conditions, isn't it?
Because if one of them returns false, and other true, we will continue reading chunks, causing that possibly one of them gets corrupted.
Shall we better allow one or the other but nut the two of them?
There was a problem hiding this comment.
Not sure I follow. If one of them returns false then the return value is false.
There was a problem hiding this comment.
Sure. But then what would happen if e.g. onResponseHeaders returns false but onHeaders returns true?
I think we are allowing them to pass both APIs at the same time? Wouldn't that maybe cause inconsistencies?
There was a problem hiding this comment.
No that's fine, if one returns false then it's paused.
| ret = false | ||
| } | ||
|
|
||
| if (this.#handler.onHeaders) { |
There was a problem hiding this comment.
Shall we prevent to have both the old and new API?
There was a problem hiding this comment.
Maybe we can check at dispatcher-base when calling dispatch. We already validate the handler, we can take advantage of validating the method as well if we want to avoid receiving both functions/method at the same time.
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status