Redirects when multiple slashes

[ErimTuzcuoglu]

Closes: #4422

• Docs
• Tests

Testing Strategy: New and existing tests pass. We can try slashed routes manually.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 69e84ce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[remix-cla-bot]

Hi @ErimTuzcuoglu,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[brophdawg11]

Is this something we need to consider for servers/runtimes other than express?

[ErimTuzcuoglu]

@brophdawg11 No, its just express specific.

[machour]

@ErimTuzcuoglu can you please fix the conflict in the contributors files? 🙏🏼
Protip: put your name in the right spot alphabetically, conflicts will be less likely to happen

[brophdawg11]

@ErimTuzcuoglu I didn't dig too deep, but was able to reproduce this using the Architect template as well? I'm wondering this is something that should be handled in a more central location, or at least should this logic go into each of the adapters. I will probably defer to one of the Remix team members here with more experience in the alternative runtimes. @jacob-ebey @mcansh?

[mcansh]

I'm wondering this is something that should be handled in a more central location

i just tried a github url and it resolved with the multiple slashes without redirecting - perhaps we follow suit and normalize while matching?

however, vercel.com does normalize and redirect, while netlify.com will 404

I didn't dig too deep, but was able to reproduce this using the Architect template as well

same with Cloudflare Pages template, but that appears to throw at the miniflare level?

[pages:err] Unhandled Promise Rejection: TypeError: Invalid URL
    at new NodeError (node:internal/errors:393:5)
    at URL.onParseError (node:internal/url:565:9)
    at new URL (node:internal/url:645:5)
    at convertNodeRequest (/Users/logan/pages-slash-test/node_modules/.pnpm/@[email protected]/node_modules/@miniflare/http-server/src/index.ts:61:15)
    at Server.<anonymous> (/Users/logan/pages-slash-test/node_modules/.pnpm/@[email protected]/node_modules/@miniflare/http-server/src/index.ts:266:36)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

[ErimTuzcuoglu]

The main problem is actually on URL constructor. It doesn't accept multiple slashes as a url and throws error. So when we removed slashes it works correctly, but for redirection I think we must make different implementations on adapters. As @mcansh said, on vercel we don't need any changes. But if we need I can check also other adapters and fix them too.

[mcansh]

The main problem is actually on URL constructor. It doesn't accept multiple slashes as a url and throws error. So when we removed slashes it works correctly, but for redirection I think we must make different implementations on adapters. As @mcansh said, on vercel we don't need any changes. But if we need I can check also other adapters and fix them too.

whats interesting is that new URL("https://github.com///") seems to be fine

[machour]

@ErimTuzcuoglu Please update other adapters as well and rebase this PR to latest dev 🙏🏼

[brophdawg11]

Closing in favor of #5336