fix: improve URL handling in getRequestURL function

[bjohansebas]

Summary

fixed #2303

What kind of change does this PR introduce?

Did you add tests for your changes?

Does this PR introduce a breaking change?

If relevant, what needs to be documented once your changes are merged or what have you already documented?

Use of AI

yes!

[changeset-bot]

🦋 Changeset detected

Latest commit: 911c292

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
webpack-dev-middleware	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.54%. Comparing base (7c658fe) to head (911c292).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2304      +/-   ##
==========================================
+ Coverage   93.52%   93.54%   +0.01%     
==========================================
  Files           3        3              
  Lines         958      960       +2     
  Branches      301      302       +1     
==========================================
+ Hits          896      898       +2     
  Misses         57       57              
  Partials        5        5              

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[alexander-akait]

It this problem only for fastlify? decodeURI is not fast, so it is bad for performance

[bjohansebas]

Yep, it's unique to Fastify, or at least I don't remember Express having the same issue. Fastify uses its own logic instead of using decodeURI fastify/middie#245

[alexander-akait]

in this case let's add a pseudo API for fastify here and implement this logic only for fastify, don't want to lose perfomance for other frameworks

[bjohansebas]

I’m not sure what steps to follow. It would be great if Fastify exposed something like req.raw on the request (since it currently doesn’t, which is a bit odd—but I’m not a Fastify expert). That way we could quickly detect it, since neither Express nor Connect have that property. However, Hono also has a getURL method, and since our logic checks getURL, it would never reach the fallback logic using req.raw.

Now the real issue is that Fastify does decode req.url, while Express and others don’t.

We could add logic like this:

if (typeof req.originalUrl !== "undefined") {
  if (req.url === req.originalUrl) {
    return req.originalUrl;
  }

  // Some frameworks can expose decoded req.url while keeping
  // encoded req.originalUrl.
  if (typeof req.url !== "undefined" && req.originalUrl.includes("%")) {
    try {
      if (decodeURI(req.originalUrl) === req.url) {
        return req.originalUrl;
      }
    } catch {
      // Ignore malformed URI sequences and fall back to req.url.
    }
  }
}

Or we could create a wrapper for Fastify, but I’m not sure how reliable it would be to correctly detect that Fastify is running underneath.

Another option would be to open an issue in @fastify/express to have them expose raw, so we can better detect Fastify internally.

What would you recommend without hurting performance too much?

I’m thinking about other ways to detect it—I’ll see what I come up with by tomorrow night

[alexander-akait]

I see the problem... Made refactor to use req.id to apply this logic only for fastify

[alexander-akait]

Or we could create a wrapper for Fastify, but I’m not sure how reliable it would be to correctly detect that Fastify is running underneath.

I think in future we can do it and implement built-in fastify support without the fastify-express layer, but to be honestly fastify is not very popular, so I think we will use fastify-express until it was supported, when it will be deprecated we will implement own wrapper

Another option would be to open an issue in @fastify/express to have them expose raw, so we can better detect Fastify internally.

It will be fine, let's try it, if it will be fixed we will revert this code and will use raw, so feel free to open an issue

[bjohansebas]

I didn’t know req.id existed, that’s new to me.