feat: redirect

[ronag]

Continuing on from: #600

Trying to figure out the API. Doing this through Agent or Pool doesn't quite make sense IMO.

[codecov-io]

Codecov Report

Merging #603 (240079c) into main (b3dd69f) will decrease coverage by 0.99%.
The diff coverage is 83.33%.

❗ Current head 240079c differs from pull request most recent head b797c93. Consider uploading reports for the commit b797c93 to get more accurate results

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
- Coverage   99.10%   98.10%   -1.00%     
==========================================
  Files          17       18       +1     
  Lines        1446     1529      +83     
==========================================
+ Hits         1433     1500      +67     
- Misses         13       29      +16     

Impacted Files	Coverage Δ
lib/client-pool.js	98.65% <50.00%> (-0.67%)	⬇️
lib/core/client.js	98.97% <50.00%> (-0.17%)	⬇️
lib/agent/index.js	89.85% <79.31%> (ø)
lib/agent/redirect.js	85.71% <85.71%> (ø)
index.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3dd69f...b797c93. Read the comment docs.

[mcollina]

We are not in agreement. Using an Agent will make adding redirect being transparent for fetch() and other high-level libraries. Adding a different API would provide a worse developer experience (however it would be a simpler implementation).

[ronag]

Hm. I agree it would be nicer to implement this just as another client and/or agent. I just don’t see how to implement this in a way that makes sense in that way.

I have some more ideas here regarding developer experience so I’ll continue working on this and see if we can reach an agreement.

[ronag]

Agent would have to return a redirect client which would have to have a reference to the agent.

Then the client needs to correctly implement all the client APIs. Many of which would not make much sense. Especially with cross origin in mind. Would have to give that more thought but it start to feel like code smell if it’s that complicated to add something this simple.

[ronag]

@mcollina PTAL. Has a change to the Agent API to enable better composition.

[ronag]

@ShogunPanda

[ShogunPanda]

@ronag I like the API. Nice work.
I would suggest to copy tests from my PR, adapt them (basically replacing RedirectPool with RedirectAgent and see it they passes.

[ronag]

I would suggest to copy tests from my PR, adapt them (basically replacing RedirectPool with RedirectAgent and see it they passes.

Yes, I was hoping we could collaborate on the tests. I'll port them over from your PR once we know whether @mcollina agrees on the approach.

[ShogunPanda]

I would suggest to copy tests from my PR, adapt them (basically replacing RedirectPool with RedirectAgent and see it they passes.

Yes, I was hoping we could collaborate on the tests. I'll port them over from your PR once we know whether @mcollina agrees on the approach.

Wonderful! ^^

[mcollina]

The approach looks good to me, I've left a comment.

[ronag]

I think this is as much as I can do at the moment. @ShogunPanda do you think you could help out with test coverage and docs? @Ethan-Arrowood maybe you could also help with docs?

Basically what we now have architecture wise is:

interface Dispatcher implements EventEmitter {
  dispatch (opts, handler)
  close ()
  destroy ()
  destroyed
  closed
  events: 'connect', 'disconnect'
}

class Agent implements Dispatcher {
}

class RedirectAgent implements Dispatcher {
}

interface Client implement Dispatcher {
  url
  pipelining
  connected
  pending
  running
  size
  busy
  events: 'drain'
}

class Pool implements Client {
}

So basically we have two traits Dispatcher implements EventEmitter and Client implements Dispatcher. Where client is just a dispatcher with a public prop interface.

[ronag]

Should we merge RedirectAgent into Agent? The functionality is kind of trivial and it would have to be explicitly enabled by passing the maxRedirects option in the constructor.

@mcollina @dnlup @ShogunPanda wdyt?

[ShogunPanda]

I agree.
Since it's a very common use case when dealing with HTTP agents, it's better to keep things simple for developer experience (in this case, not having to remember to use a different agent for a comon case).
And since it's protected by maxRedirects I don't see performance problems.

Also, I have to agree since it was similar to my original proposal in #553 (comment). Just joking. Lol.

[mcollina]

Good for me!

[ronag]

Done!

[ronag]

This is starting to feel pretty solid. Just waiting for docs + test coverage. @Ethan-Arrowood @ShogunPanda.

[ShogunPanda]

@ronag @mcollina

I've sent #621 which brings coverage back to 100%.
It also fixes couple of minor stuffs. Please let me know what you think about it.

[ShogunPanda]

Also, final note. Once this get merged, I'll start working on #615.