fix: respect fragment in trailing slash utils

[manniL]

Resolves #174

It also will not change the trailing slash behavior if there is a protocol other than http or https present (e.g. mailto:)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11f161e) 95.11% compared to head (1ab878f) 95.27%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
+ Coverage   95.11%   95.27%   +0.15%     
==========================================
  Files           7        7              
  Lines         840      867      +27     
  Branches      180      187       +7     
==========================================
+ Hits          799      826      +27     
  Misses         41       41              

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

[pi0]

I am concerned about the performance implications of doing this (as this is a very low level util). Maybe we can update TRAILING_SLASH_RE and add another for !queryParameters branch to use reges based testing?

[manniL]

I understand. Will ping you and update with a better approach ensuring it won't cause much overhead 👍

[manniL]

Two questions:

Would you prefer another argument (e.g. fragment = false) or add fragment support to the queryParameters arg?
I have the feeling that another arg would be cleaner but also would add more complexity (4 combinations to consider instead of 2)

---

Do you think the perf implication applies to withoutTrailingSlash and withTrailingSlash too, or only hasTrailingSlash?

---

(updated my PR to add the functionality to the queryParameters arg but happy to add another param if you think it is necessary.

[manniL]

Another thing: Trailing slash should only applied if it is http/https protocol or relative link. (so no tel/mailto protocol)

[manniL]

@pi0 I've updated the PR to include protocol handling and also wrapped fragment handling so perf overhead is reduced

Do you have further recommendations?
Should I bench the changes?

Benchmark

TL;DR - Perf overhead was only the protocol checking. I've added another param for it in the PR.
Other tests have no significant overhead.

Current main

withTrailingSlash, no fragment in url x 105,797,840 ops/sec ±1.41% (93 runs sampled)
withTrailingSlash fragment in url x 125,971,442 ops/sec ±1.53% (92 runs sampled)

withTrailingSlash, no fragment in url, queryParams true x 51,973,496 ops/sec ±1.33% (93 runs sampled)
withTrailingSlash, fragment in url, queryParams true x 51,607,080 ops/sec ±1.52% (92 runs sampled)

---

hasTrailingSlash, no fragment in url x 97,867,504 ops/sec ±4.83% (81 runs sampled)
hasTrailingSlash fragment in url x 82,091,568 ops/sec ±2.42% (90 runs sampled)

hasTrailingSlash, no fragment in url, queryParams true x 45,007,719 ops/sec ±1.53% (90 runs sampled)
hasTrailingSlash, fragment in url, queryParams true x 56,544,849 ops/sec ±1.33% (94 runs sampled)

(protocol options does not exist there, omitting these)

This PR

withTrailingSlash, no fragment in url x 108,017,450 ops/sec ±1.05% (97 runs sampled)
withTrailingSlash fragment in url x 126,070,571 ops/sec ±1.26% (95 runs sampled)

withTrailingSlash, no fragment in url, queryParams true x 51,390,197 ops/sec ±1.03% (92 runs sampled)
withTrailingSlash, fragment in url, queryParams true x 51,151,668 ops/sec ±1.64% (96 runs sampled)

withTrailingSlash, no fragment in url, queryParams true, protocol true x 32,238,491 ops/sec ±0.94% (95 runs sampled)
withTrailingSlash, fragment in url, queryParams true, protocol true x 32,436,897 ops/sec ±0.96% (95 runs sampled)

withTrailingSlash, no fragment in url, queryParams false, protocol true x 47,730,336 ops/sec ±1.00% (95 runs sampled)
withTrailingSlash fragment in url, queryParams false, protocol true x 49,205,926 ops/sec ±1.27% (95 runs sampled)

---

hasTrailingSlash, no fragment in url x 117,062,355 ops/sec ±1.01% (95 runs sampled)
hasTrailingSlash fragment in url x 90,473,699 ops/sec ±4.69% (67 runs sampled)

hasTrailingSlash, no fragment in url, queryParams true x 42,962,486 ops/sec ±1.99% (88 runs sampled)
hasTrailingSlash, fragment in url, queryParams true x 54,536,116 ops/sec ±1.48% (95 runs sampled)

Code for benches

import Benchmark from "benchmark"
import { withTrailingSlash, hasTrailingSlash } from "ufo"

const { log } = console


const suite = new Benchmark.Suite()

suite.on("cycle", (event) => {
  log(String(event.target))
})

suiteHelper('withTrailingSlash', withTrailingSlash)
suiteHelper('hasTrailingSlash', hasTrailingSlash, false)

suite.run()

function suiteHelper(name, fn, hasProtocol = true) {
  suite.add(`${name}, no fragment in url`, () => {
    fn('/abc/')
  })

  suite.add(`${name} fragment in url`, () => {
    fn('/abc/#abc')
  })

  suite.add(`${name}, no fragment in url, queryParams true`, () => {
    fn('/abc/?test=true', true)
  })

  suite.add(`${name}, fragment in url, queryParams true`, () => {
    fn('/abc/?test=true#abc', true)
  })

  if (hasProtocol) {
    suite.add(`${name}, no fragment in url, queryParams true, protocol true`, () => {
      fn('/abc/?test=true', true, true)
    })

    suite.add(`${name}, fragment in url, queryParams true, protocol true`, () => {
      fn('/abc/?test=true#abc', true, true)
    })


    suite.add(`${name}, no fragment in url, queryParams false, protocol true`, () => {
      fn('/abc/', false, true)
    })

    suite.add(`${name} fragment in url, queryParams false, protocol true`, () => {
      fn('/abc/#abc', false, true)
    })
  }
}

pnpm i -D benchmark && pnpm build && node ./bench.mjs

[manniL]

Last changes fix the issue that a sole fragment should never be changed

/#abc and #abc should stay as is because the semantics are different (current page vs. root page!)