URL's on incoming requests

[wesleytodd]

Initial context: nodejs/node#12682

I think the new api's should use URL as the basis for req.url (or whatever equivalent we expose). As you can see from the thread above, there is some question about how today these are relative urls and the new spec does not support that. My thought, and what I think we need to discuss, is that the "base" should be derived from the incoming request headers and fall back to the server address.

If we come to an agreement on this I am happy to write up a more detailed description. Agenda added so we can discuss in the next meeting as well.

[mcollina]

I'm highly -1 on using new URL() for req.url. It's extremely slow and inefficient due to the host and protocol parsing.

[ronag]

I usually use, https://www.npmjs.com/package/request-target. Maybe worth looking into?

[ronag]

https://github.com/nodejs/next-10/blob/master/VALUES_AND_PRIORITIZAION.md might also be worth considering, i.e. "Performance" vs "Web API compatibility".

[Ethan-Arrowood]

could we do something like req.url returns the faster solution and then req.url() would return the new URL() version? This way users have to opt-in to the performance decrease if they want it?

Additionally, could we look into improving host and protocol parsing? Or is has that already been attempted?

[wesleytodd]

This is why I was strongly questioning adding that (nodejs/node#35323) in yesterdays meeting.

The perf is a big concern I was not aware of (do we have benchmarks on this, or details on why?), but is also a good reason to push back on the web api compat technical value issue.

[ronag]

do we have benchmarks on this, or details on why and what parts are slow?

From https://www.npmjs.com/package/request-target

$ npm run benchmark
legacy url.parse() x 371,681 ops/sec ±0.88% (297996 samples)
whatwg new URL() x 58,766 ops/sec ±0.3% (118234 samples)
request-target x 552,748 ops/sec ±0.54% (344809 samples)

[wesleytodd]

I hadn't clicked your link, but yeah this looks good. I will dig into what it is doing, thanks for the link!

[wesleytodd]

Well I very quickly ended on this line, and I am fairly confident we will be able to find a better (and even better perf) way than a big regex.

But this leads me to my next question: do we really want to deviate from the existing URL apis?

Seems like this would add "yet another url" to node core, right? Maybe we could get fast parsing but also expose an api which looks like a URL instance, but this gets us into a similar situation of "its a stream but not". Anyway, just thinking out loud on this.

[styfle]

It's extremely slow and inefficient due to the host and protocol parsing.

Is the slow performance of new URL() inherent to the way its spec'ed or just the Node.js implementation?

If its the latter, then this would be a good excuse to re-evaluate the implementation to squeeze out better performance.

[styfle]

Overall, I think providing a way to retrieve req.url as a URL would be a great addition to Node core because the URL object is the future, url.parse() is a legacy of the past, before there was a standard.

Using standard JS URL objects allows code to be shared between the browser, Node.js, etc.

One of the big foot-guns today is parsing url query string params. Consider this code:

const { parse } = require('url');
const handler = (req, res) => {
  let { pathname = '/', query = {} } = parse(req.url || '', true);
  const { user = '' } = query;
  console.log(user.toLowerCase());
}

This works fine with https://example.com/api?user=foo but throws when two parameters with the same name are detected https://example.com/api?user=foo&user=bar because user suddenly becomes an array instead of a string.

Consider this similar code using URL:

const handler = (req, res) => {
  const url = new URL(req.url, 'https://example.com');
  const user = url.searchParams.get('user') || '';
  console.log(user.toLowerCase());
}

This bug doesn't happen with URL because it separates getAll() vs get(). Now we won't crash in production.

My workaround for the time being looks like this:

export function getURL(req) {
  const proto = req.headers['x-forwarded-proto'] || 'https';
  const host = req.headers['x-forwarded-host'] || req.headers.host || 'example.com';
  return new URL(req.url || '/', `${proto}://${host}`);
}

[ljharb]

It makes sense to me to offer both (obviously under different names) - ie, req.url as the string, and req.getURL() as the lazily-computed-but-memoized URL object.

[wesleytodd]

It makes sense to me to offer both

I think before we make any decisions on this, we need to know if we can help fix the perf issues of URL. If we cannot do that, then I think we should consider alternate api's to expose both. If not, I think one api is best.

My workaround for the time being looks like this:

@styfle There are security concerns here, but if we "do it right" I think it is a reasonable approach (if you know those headers can only be set by trusted proxies, you should be fine). The added support for the meta headers would also change this a bit, but not in a bad way.

Also, we would default them to the values computed from server.address() I think.