replace fast-uri url parser with URL contructor

[synapse]

Fixes #141

N.B. this fix replaces the current URIComponent with URL.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[gurgunday]

Looks good to me, you can also remove the dependency I believe

[jsumners]

I think there was an unanswered question in the referenced issue. Has that been addressed?

[gurgunday]

Oh, that’s true, it might need a benchmark

[mcollina]

can you avoid the breaking change?

[synapse]

To avoid it, would mean converting the URL to an URIComponent object.
I can map it 1-to-1 and return what we previously had, but there's a s mall caveat where fast-uri previously also had a prop on the URIComponent called userinfo which returned something like this : "user:pass".
By default the URL does not have it, it would require creating ourselves just like as fast-uri does it.

[mcollina]

nevermind

[mcollina]

We should really have a benchmark to check if the breakage is worth the change.

[gurgunday]

Just blocking temporarily until we get a general benchmark, will try to look into it, but feel free to attach one if you can

[synapse]

Here are the benchmarks:

Code used

const fastUri = require('fast-uri');
const benchmark = require('benchmark')

const url = 'uri://user:pass@example.com:123/one/two.three?q1=a1&q2=a2#body';

new benchmark.Suite()
  .add('fast-uri', function () {
    fastUri.parse(url)
  })
  .add('url', function () {
    new URL(url)
  })
  .on('cycle', function (event) {
    console.log(String(event.target));
  })
  .on('complete', function () {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
  })
  .run({ async: true });

Results run on Node version v20.11.1 LTS:

fast-uri x 2,588,130 ops/sec ±1.30% (95 runs sampled)
url x 2,910,301 ops/sec ±0.87% (93 runs sampled)
Fastest is url

fast-uri x 2,625,725 ops/sec ±0.36% (98 runs sampled)
url x 3,026,747 ops/sec ±0.33% (94 runs sampled)
Fastest is url

fast-uri x 2,651,826 ops/sec ±0.41% (98 runs sampled)
url x 3,029,741 ops/sec ±0.51% (92 runs sampled)
Fastest is url

Results run on Node version v18.19.1:

fast-uri x 2,522,467 ops/sec ±0.78% (97 runs sampled)
url x 2,730,338 ops/sec ±0.47% (94 runs sampled)
Fastest is url

fast-uri x 2,511,920 ops/sec ±0.73% (96 runs sampled)
url x 2,664,369 ops/sec ±0.45% (98 runs sampled)
Fastest is url

fast-uri x 2,486,801 ops/sec ±0.44% (97 runs sampled)
url x 2,700,008 ops/sec ±0.40% (96 runs sampled)
Fastest is url

[Uzlopak]

Performance on node18?

[gurgunday]

lgtm

@Uzlopak, for this comment, I mean instantiation is what matters anyway, no? Ada returns the full object as far as I know, and property access is cheap; are these property getters?

[mcollina]

As shown in other issues, it's important to actually benchmark the actual module, not the function in isolation.

[synapse]

You're absolutely right @mcollina I am already working on that as well. I'll also post the results soon.

[synapse]

Here is the benchmarks using it in fastify:

code used:

const benchmark = require('benchmark')
const Fastify = require('fastify')
const http = require('node:http')
const pluginFast = require('./plugin-fast');
const pluginUrl = require('../plugin');

async function getFastify(plugin, port) {
  const fastify = Fastify()
  fastify
    .register(plugin)
    .after((err) => {
      if (err) console.error(err)
    })
  
  fastify.get("/one", (req, reply) => {
    const uriData = req.urlData()
    reply.send()
  })

  try {
    await fastify.listen({ port })    
  } catch (err) {
    process.exit(1)
  }

  return fastify;
}

const bench = async () => {
  const fastifyPluginOld = await getFastify(pluginFast, 3201);
  const fastifyPluginNew = await getFastify(pluginUrl, 3202);

  new benchmark.Suite()
    .add('fast-uri', async function () {
      await fastifyPluginOld.inject({
        method: "GET",
        url: "/one?a=b&c=d#foo",
      })
    })
    .add('url', async function () {
      await fastifyPluginNew.inject({
            method: "GET",
            url: "/one?a=b&c=d#foo",
          })
    })
    .on('cycle', function (event) {
      console.log(String(event.target));
    })
    .on('complete', function () {
      console.log('Fastest is ' + this.filter('fastest').map('name'));
      fastifyPluginOld.close()
      fastifyPluginNew.close()
    })
    .run({async: true});
}

bench();

Results run on Node version v20.11.1 LTS:

fast-uri x 1,171,424 ops/sec ±8.73% (58 runs sampled)
url x 1,172,971 ops/sec ±7.79% (49 runs sampled)
Fastest is url,fast-uri

Results run on Node version v18.19.1:

fast-uri x 1,388,169 ops/sec ±18.89% (50 runs sampled)
url x 1,366,116 ops/sec ±20.89% (37 runs sampled)
Fastest is fast-uri,url

[mcollina]

Given it has no significant gain, I don't think changing the module API is worth it.

[kibertoad]

@mcollina reducing amount of dependencies is not a good thing on its own?

[mcollina]

But it's not. fast-uri is a dependency of fastify and it would keep being one.

[jsumners]

Given lack of movement, and the conclusions by Matteo, I am closing this.