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

feat: add new dispatch compose #2826

Merged
merged 25 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
560d35b
feat: add new dispatch compose
metcoder95 Feb 23, 2024
5337ea0
fix: review
metcoder95 Feb 25, 2024
8019e59
revert: linting
metcoder95 Feb 25, 2024
1d54170
docs: add documentation
metcoder95 Feb 25, 2024
051caa1
Merge remote-tracking branch 'origin/main' into feat/compose-dispatch
metcoder95 Feb 25, 2024
b45ecad
fix: smaller tweaks to proxy interceptor
metcoder95 Feb 28, 2024
9048eb5
test: fix tests for proxy
metcoder95 Feb 28, 2024
7656e1b
refactor: expose interceptor as is
metcoder95 Feb 29, 2024
9f0631e
Merge branch 'main' into feat/compose-dispatch
metcoder95 Feb 29, 2024
5588a1d
test: add testing for retry
metcoder95 Mar 1, 2024
04c8acb
refactor: rewrite interceptors
metcoder95 Mar 3, 2024
8ac252d
refactor: proxy interceptor
metcoder95 Mar 3, 2024
8c8c064
feat: redirect interceptor
metcoder95 Mar 3, 2024
9d1a6c0
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 3, 2024
0fd9ea7
refactor: change the compose behaviour
metcoder95 Mar 6, 2024
53f4cfa
docs: update docs
metcoder95 Mar 6, 2024
6580d84
test: add testing for compose
metcoder95 Mar 6, 2024
1d987a2
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 6, 2024
d66530f
feat: composed dispatcher
metcoder95 Mar 8, 2024
b932ad2
docs: adjust documentation
metcoder95 Mar 8, 2024
20d3a33
refactor: apply review
metcoder95 Mar 8, 2024
2d59acc
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 8, 2024
2f5982e
docs: tweaks
metcoder95 Mar 8, 2024
eb065f7
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 12, 2024
620f9bc
feat: drop proxy
metcoder95 Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions docs/docs/api/Dispatcher.md
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,131 @@ try {
}
```

### `Dispatcher.compose(interceptors[, interceptor])`
mcollina marked this conversation as resolved.
Show resolved Hide resolved

Compose a new dispatcher from the current dispatcher and the given interceptors.

> _Notes_:
> - The order of the interceptors is important. The first interceptor will be the first to be called.
> - It is important to note that the `interceptor` function should return a `Dispatcher` instance.
> - Any fork of the chain of `interceptors` can lead to unexpected results, it is important that an interceptor returns a `Dispatcher` instance that forwards the request to the next interceptor in the chain.

Arguments:

* **interceptors** `Interceptor[]`: It is an array of `Interceptor` functions passed as only argument, or several interceptors passed as separate arguments.

Returns: `Dispatcher`.

#### Parameter: `Interceptor`

A function that takes a `Dispatcher` instance and returns a `Dispatcher` instance.

#### Example 1 - Basic Compose
mcollina marked this conversation as resolved.
Show resolved Hide resolved

```js
import { RedirectHandler, Dispatcher } from 'undici'

class RedirectDispatcher extends Dispatcher {
#opts
#dispatcher

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RedirectHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}

const redirectInterceptor = dispatcher => new RedirectDispatcher(dispatcher, opts)

const client = new Client('http://localhost:3000')
.compose(redirectInterceptor)
```

#### Example 2 - Chained Compose

```js
import { RedirectHandler, Dispatcher, RetryHandler } from 'undici'

class RedirectDispatcher extends Dispatcher {
#opts
#dispatcher

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RedirectHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}

class RetryDispatcher extends Dispatcher {
#dispatcher
#opts

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RetryHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}


const redirectInterceptor = dispatcher => new RedirectDispatcher(dispatcher, opts)
const retryInterceptor = dispatcher => new RetryDispatcher(dispatcher, opts)

const client = new Client('http://localhost:3000')
.compose(redirectInterceptor)
.compose(retryInterceptor)
```

## Instance Events

### Event: `'connect'`
Expand Down
68 changes: 68 additions & 0 deletions docs/docs/api/Interceptors.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Interceptors
Copy link
Member

Choose a reason for hiding this comment

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

Generically I don't think we need a special page for the interceptors. They are more confusing than anything else. Let's only have Dispatchers.

Having the use specify (dispatcher) => new MyDispatcher(dispatcher, myOpts) is not a problem for most devs. These helpers only add noise.


Undici provides a way to intercept requests and responses using interceptors.

Interceptors are a way to modify the request or response before it is sent or received by the original dispatcher, apply custom logic to a network request, or even cancel the request, connect through a proxy for the origin, etc.

Within Undici there are a set of pre-built that can be used, on top of that, you can create your own interceptors.

## Pre-built interceptors

### `proxy`

The `proxy` interceptor allows you to connect to a proxy server before connecting to the origin server.

It accepts the same arguments as the [`ProxyAgent` constructor](./ProxyAgent.md).

#### Example - Basic Proxy Interceptor

```js
const { Client, interceptors } = require("undici");
const { proxy } = interceptors;

const client = new Client("http://example.com");

client.compose(proxy("http://proxy.com"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const client = new Client("http://example.com");
client.compose(proxy("http://proxy.com"));
const client = new Client("http://example.com").compose(proxy("http://proxy.com"));

```

### `redirect`

The `redirect` interceptor allows you to customize the way your dispatcher handles redirects.

It accepts the same arguments as the [`RedirectHandler` constructor](./RedirectHandler.md).

#### Example - Basic Redirect Interceptor

```js
const { Client, interceptors } = require("undici");
const { redirect } = interceptors;

const client = new Client("http://example.com");

client.compose(redirect({ maxRedirections: 3, throwOnMaxRedirects: true }));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const client = new Client("http://example.com");
client.compose(redirect({ maxRedirections: 3, throwOnMaxRedirects: true }));
const client = new Client("http://example.com").compose(redirect({ maxRedirections: 3, throwOnMaxRedirects: true }));

```

### `retry`

The `retry` interceptor allows you to customize the way your dispatcher handles retries.

It accepts the same arguments as the [`RetryHandler` constructor](./RetryHandler.md).

#### Example - Basic Redirect Interceptor

```js
const { Client, interceptors } = require("undici");
const { retry } = interceptors;

const client = new Client("http://example.com");

client.compose(
retry({
maxRetries: 3,
minTimeout: 1000,
maxTimeout: 10000,
timeoutFactor: 2,
retryAfter: true,
})
);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const client = new Client("http://example.com");
client.compose(
retry({
maxRetries: 3,
minTimeout: 1000,
maxTimeout: 10000,
timeoutFactor: 2,
retryAfter: true,
})
);
const client = new Client("http://example.com").compose(
retry({
maxRetries: 3,
minTimeout: 1000,
maxTimeout: 10000,
timeoutFactor: 2,
retryAfter: true,
})
);

```
1 change: 1 addition & 0 deletions docs/docsify/sidebar.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* [Agent](/docs/api/Agent.md "Undici API - Agent")
* [ProxyAgent](/docs/api/ProxyAgent.md "Undici API - ProxyAgent")
* [RetryAgent](/docs/api/RetryAgent.md "Undici API - RetryAgent")
* [Interceptors](/docs/api/Interceptors.md "Undici API - Interceptors")
* [Connector](/docs/api/Connector.md "Custom connector")
* [Errors](/docs/api/Errors.md "Undici API - Errors")
* [EventSource](/docs/api/EventSource.md "Undici API - EventSource")
Expand Down
5 changes: 5 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ module.exports.RetryHandler = RetryHandler
module.exports.DecoratorHandler = DecoratorHandler
module.exports.RedirectHandler = RedirectHandler
module.exports.createRedirectInterceptor = createRedirectInterceptor
module.exports.interceptors = {
proxy: require('./lib/interceptor/proxy'),
redirect: require('./lib/interceptor/redirect'),
retry: require('./lib/interceptor/retry')
}

module.exports.buildConnector = buildConnector
module.exports.errors = errors
Expand Down
27 changes: 26 additions & 1 deletion lib/dispatcher/dispatcher.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
'use strict'

const EventEmitter = require('node:events')

const kDispatcherVersion = Symbol.for('undici.dispatcher.version')
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same simbol we use for the global? I think we should use the same.


class Dispatcher extends EventEmitter {
[kDispatcherVersion] = 1

dispatch () {
throw new Error('not implemented')
}
Expand All @@ -14,6 +17,28 @@ class Dispatcher extends EventEmitter {
destroy () {
throw new Error('not implemented')
}

compose (...args) {
// So we handle [interceptor1, interceptor2] or interceptor1, interceptor2, ...
const interceptors = Array.isArray(args[0]) ? args[0] : args
let dispatcher = this
for (const interceptor of interceptors) {
if (interceptor == null) {
continue
}

if (typeof interceptor !== 'function') {
ronag marked this conversation as resolved.
Show resolved Hide resolved
throw new Error('invalid interceptor')
}

dispatcher = interceptor(dispatcher) ?? dispatcher
ronag marked this conversation as resolved.
Show resolved Hide resolved

if (dispatcher[kDispatcherVersion] !== 1) {
throw new Error('invalid dispatcher')
}
}
return dispatcher
}
}

module.exports = Dispatcher
39 changes: 39 additions & 0 deletions lib/interceptor/proxy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict'

const { InvalidArgumentError } = require('../core/errors')
const ProxyAgent = require('../dispatcher/proxy-agent')
const DispatcherBase = require('../dispatcher/dispatcher-base')

class ProxyInterceptor extends DispatcherBase {
Copy link
Member

Choose a reason for hiding this comment

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

Please integrate this inside the ProxyAgent. We shouldn't have special classes for these.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should extend Dispatcher, not DispatcherBase.

Copy link
Member Author

@metcoder95 metcoder95 Feb 28, 2024

Choose a reason for hiding this comment

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

Please integrate this inside the ProxyAgent. We shouldn't have special classes for these.

Sure, let me double-check this part as I'm not totally confident about it, I'd try to first assess if the chain of dispatcher works as expected before trying to port it to the ProxyAgent

constructor (dispatcher, opts) {
super()
this.dispatcher = dispatcher
this.agent = new ProxyAgent(opts)
}

dispatch (opts, handler) {
return this.agent.dispatch(opts, handler)
}

close () {
return this.dispatcher.close().then(() => this.agent.close())
}
}

module.exports = opts => {
if (typeof opts === 'string') {
opts = { uri: opts }
}

if (!opts || (!opts.uri && !(opts instanceof URL))) {
throw new InvalidArgumentError('Proxy opts.uri or instance of URL is mandatory')
}

if (opts.auth && opts.token) {
throw new InvalidArgumentError(
'opts.auth cannot be used in combination with opts.token'
)
}
ronag marked this conversation as resolved.
Show resolved Hide resolved

return dispatcher => new ProxyInterceptor(dispatcher, opts)
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't an interceptor?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was referring to when I suggested that ProxyAgent works differently 😅
It requires to hope over the whole dispatch, as it forwards the request to the proxy instead of the intended dispatch.
It does not respects the previous dispatcher, so if proxy is used, it should be placed at the very beginning of the chain.

I couldn't think of any other way without re-working the ProxyAgent, but totally open to suggestions

Copy link
Member

Choose a reason for hiding this comment

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

I will have to investigate this

Copy link
Member

@ronag ronag Mar 10, 2024

Choose a reason for hiding this comment

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

It sounds to me like this should not be an interceptor.

Copy link
Member

Choose a reason for hiding this comment

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

Let's skip this for now so we can land.

44 changes: 44 additions & 0 deletions lib/interceptor/redirect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict'

const { InvalidArgumentError } = require('../core/errors')
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
const Dispatcher = require('../dispatcher/dispatcher-base')
const RedirectHandler = require('../handler/RedirectHandler')

class RedirectDispatcher extends Dispatcher {
Copy link
Member

Choose a reason for hiding this comment

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

This class is useful as its own export.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add it as a RedirectAgent

#opts
#dispatcher

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RedirectHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}

module.exports = opts => {
if (opts?.maxRedirections == null || opts?.maxRedirections === 0) {
return null
}

if (!Number.isInteger(opts.maxRedirections) || opts.maxRedirections < 0) {
throw new InvalidArgumentError('maxRedirections must be a positive number')
}

return dispatcher => new RedirectDispatcher(dispatcher, opts)
}
35 changes: 35 additions & 0 deletions lib/interceptor/retry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict'

const Dispatcher = require('../dispatcher/dispatcher-base')
const RetryHandler = require('../handler/RetryHandler')

class RetryDispatcher extends Dispatcher {
#dispatcher
#opts

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RetryHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}

module.exports = opts => {
return dispatcher => new RetryDispatcher(dispatcher, opts)
}
Loading
Loading