A few questions

[szmarczak]

1. We can provide connect function or connect options but not both. Why?
2. Why options.origin and not options.protocol, options.host and options.port?

3. undici/lib/core/request.js

   Lines 100 to 127 in aebbb5d

   	if (typeof handler.onConnect !== 'function') {
   	throw new InvalidArgumentError('invalid onConnect method')
   	}
   	if (typeof handler.onError !== 'function') {
   	throw new InvalidArgumentError('invalid onError method')
   	}
   	if (typeof handler.onBodySent !== 'function' && handler.onBodySent !== undefined) {
   	throw new InvalidArgumentError('invalid onBodySent method')
   	}
   	if (this.upgrade || this.method === 'CONNECT') {
   	if (typeof handler.onUpgrade !== 'function') {
   	throw new InvalidArgumentError('invalid onUpgrade method')
   	}
   	} else {
   	if (typeof handler.onHeaders !== 'function') {
   	throw new InvalidArgumentError('invalid onHeaders method')
   	}
   	if (typeof handler.onData !== 'function') {
   	throw new InvalidArgumentError('invalid onData method')
   	}
   	if (typeof handler.onComplete !== 'function') {
   	throw new InvalidArgumentError('invalid onComplete method')
   	}

   these checks are missing in RedirectHandler
4. Isn't

   undici/lib/core/request.js

   Line 130 in aebbb5d

   this.servername = util.getServerName(this.host)

   moot because

   undici/lib/core/request.js

   Line 77 in aebbb5d

   this.host = null

5. nitpick:

   undici/lib/client.js

   Line 1096 in aebbb5d

   if (hostname.startsWith('[')) {

   is slower by ~20% than hostname[0] === '['

6. undici/lib/client.js

   Lines 1397 to 1399 in aebbb5d

   	// TODO: An HTTP/1.1 user agent MUST NOT preface
   	// or follow a request with an extra CRLF.
   	// https://tools.ietf.org/html/rfc7230#section-3.5

   can be removed because ee64b39?
7. Can we avoid object allocation somehow? I think that should improve the performance as well. I mean

   undici/lib/client.js

   Lines 1439 to 1449 in aebbb5d

   	writeStream({ client, request, socket, contentLength, header, expectsPayload })
   	} else if (util.isStrictIterable(body)) {
   	writeIterable({ client, request, socket, contentLength, header, expectsPayload })
   	} else {
   	assert(false)
   	}
   	return true
   	}
   	function writeStream ({ client, request, socket, contentLength, header, expectsPayload }) {

8. undici/lib/handler/redirect.js

   Line 28 in aebbb5d

   // TODO (fix): Write a comment why this is needed?

   this reassures us that there is no body sent on redirect
9. I wonder if

   undici/index.js

   Lines 40 to 65 in aebbb5d

   	if (typeof opts === 'function') {
   	handler = opts
   	opts = null
   	}
   	if (!url || (typeof url !== 'string' && typeof url !== 'object' && !(url instanceof URL))) {
   	throw new InvalidArgumentError('invalid url')
   	}
   	if (opts != null && typeof opts !== 'object') {
   	throw new InvalidArgumentError('invalid opts')
   	}
   	if (opts && opts.path != null) {
   	if (typeof opts.path !== 'string') {
   	throw new InvalidArgumentError('invalid opts.path')
   	}
   	url = new URL(opts.path, util.parseOrigin(url))
   	} else {
   	if (!opts) {
   	opts = typeof url === 'object' ? url : {}
   	}
   	url = util.parseURL(url)
   	}

   can be removed

[mcollina]

The vast majority of those are PRs that could be sent 🙏❤️

[ronag]

1. Because if you provide a method you can save all params in the closure. Thus you don't need both.
2. Simplicity I guess...
3. PR. A validateHandler util?
4. host is set through processHeader
5. PR
6. PR
7. I think V8 might handle that already. Don't think it makes much of a diff.
8. PR
9. Why you think that?

[joaopedrocampos]

Hey guys, decided to try the fixes that @ronag said that needed a PR, in this case, the items 3 and 5

• Change startsWith for [0] ===
• Create an util function that validates handler (I am not sure if the solution I did is the best but I think this is a better discussion for the PR)

PR: #894

[szmarczak]

9. Why you think that?

Sorry, my bad. I thought the code was duplicated somewhere else.

4. host is set through processHeader

Right 👍🏼

1. Because if you provide a method you can save all params in the closure. Thus you don't need both.

Hmm... makes sense.

Thanks for answering!