Sandbox::https://github.com/nodejs/undici/pull/2826

metcoder95 · 2024-02-23T10:54:33Z

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

ghost · 2024-02-23T10:55:19Z

Next, time to fight with tests and adapt it

ghost

I think just making it available with docs is sufficient here and then we can apply it in next with a revision of my PR.

ghost

Missing docs and tests

ghost · 2024-02-25T09:45:27Z

I would also like us to have some kind of benchmark to this pattern to make sure it doesn't add too much overhead.

Co-authored-by: Matteo Collina <hello@matteocollina.com>

ghost · 2024-02-25T11:56:38Z

@ronag, did the Proxy worked for you?
I'm trying to add some testing, and it seems it will require several pieces to be changed; especially around the way we handle withe dispatch usage for connect

ghost · 2024-02-25T12:08:28Z

All tests passed. So I think so?

ghost · 2024-02-25T12:20:35Z

Well, that's because we I've not pushed the tests for the compose feature; still in my machine and constantly failing, on top I forgot to add the extra dispatcher on the constructor for the Proxy.

But its fine, I just wanted to be sure I had the whole picture. Most likely I'll put back the Agent where it was, and copy/paste and adapt a new copy of the Agent proxy to take into account the new dispatcher arg

ghost · 2024-02-28T10:27:01Z

Codecov Report

❌ Patch coverage is 49.53271% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.48%. Comparing base (0b5a451) to head (620f9bc).
⚠️ Report is 825 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/interceptor/redirect.js	16.66%	20 Missing ⚠️
lib/interceptor/retry.js	21.05%	15 Missing ⚠️
lib/dispatcher/dispatcher.js	74.46%	12 Missing ⚠️
lib/dispatcher/client.js	46.15%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2826      +/-   ##
==========================================
- Coverage   93.68%   93.48%   -0.20%     
==========================================
  Files          87       89       +2     
  Lines       23973    24102     +129     
==========================================
+ Hits        22458    22531      +73     
- Misses       1515     1571      +56

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ghost · 2024-02-28T10:28:49Z

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


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"));

ghost · 2024-02-28T10:29:00Z

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


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 }));

ghost · 2024-02-28T10:29:16Z

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


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,

})

);

ghost · 2024-02-28T10:55:58Z

+const ProxyAgent = require('../dispatcher/proxy-agent')
+const DispatcherBase = require('../dispatcher/dispatcher-base')
+
+class ProxyInterceptor extends DispatcherBase {


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

I think this should extend Dispatcher, not DispatcherBase.

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

ghost · 2024-02-28T10:56:53Z

+const Dispatcher = require('../dispatcher/dispatcher-base')
+const RedirectHandler = require('../handler/RedirectHandler')
+
+class RedirectDispatcher extends Dispatcher {


This class is useful as its own export.

We can add it as a RedirectAgent

ghost · 2024-02-28T10:58:31Z

@@ -0,0 +1,68 @@
+# Interceptors


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.

ghost · 2024-02-28T11:11:09Z

-
 const EventEmitter = require('node:events')

+const kDispatcherVersion = Symbol.for('undici.dispatcher.version')


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

ghost

I slightly disagree with @mcollina. I don't see any point in exposing the dispatcher classes themselves (why would you extend them)? Rather I would prefer exposing curry-able factory methods. That also makes the distinction between composable and non composable dispatcher clearer.

ghost · 2024-02-29T21:13:31Z

I slightly disagree with @mcollina. I don't see any point in exposing the dispatcher classes themselves (why would you extend them)? Rather I would prefer exposing curry-able factory methods. That also makes the distinction between composable and non composable dispatcher clearer.

I pushed the change to put both options over the table and chat about which one we would like to follow.
On this one, I agree with @ronag the dispatcher classes made for the interceptors might be better to keep internal, as we are looking to introduce the concept of composable-dispatcher. Otherwise it kind of becomes more like building Agents we already have the concept of

ghost · 2024-03-10T18:50:37Z

Actually ProxyAgent might make sense as an interceptor if [kClient] is the outer dispatcher.

Your initial suggestion was doing that somehow, but that will mean break the current ProxyAgent; wdyt if we drop the ProxyInterceptor and we migrate the ProxyAgent to also be an interceptor in the next major?

It doesn't necessarily break it if the interceptor just passed a client factory that return the outer dispatcher?

ghost · 2024-03-10T18:59:32Z

Actually ProxyAgent might make sense as an interceptor if [kClient] is the outer dispatcher.

Your initial suggestion was doing that somehow, but that will mean break the current ProxyAgent; wdyt if we drop the ProxyInterceptor and we migrate the ProxyAgent to also be an interceptor in the next major?

It doesn't necessarily break it if the interceptor just passed a client factory that return the outer dispatcher?

Right, that's true; but also we use the client.connect to obtain the socket from the Proxy and pass it down to the connector for doing an upgrade if necessary.
We would need to provide a wrapper around that as well; but its true might be doable within minor

ghost · 2024-03-11T06:57:36Z

We can also just skip making it an interceptor. I'm fine with that.

ghost · 2024-03-13T08:58:49Z

Dropped the proxy interceptor 👍

ghost

lgtm

feat: add new dispatch compose

560d35b

ronag approved these changes Feb 23, 2024

View reviewed changes