vuejs / router

🚦 The official router for Vue.js
https://router.vuejs.org/
MIT License
3.87k stars 1.18k forks source link

slashes should not be encoded for catch all regexes #1638

Open keith0305 opened 1 year ago

keith0305 commented 1 year ago

Version

4.1.6

Reproduction link

codesandbox.io

Steps to reproduce

  1. Load the codesandbox.io project using the link provided.
  2. Click on About page.
  3. Click on the "Add hash" button, this will call router.replace({ hash: '#something' }).
  4. Notice that vue-router correctly appends the # in the url. (https://domain.com/about.html#yippy).
  5. Click on Deep About page.
  6. Click on the "Add hash" button.
  7. Notice that the url becomes https://domain.com/test%2Fdeep%2Fabout.html#yaya.

What is expected?

Url to become https://domain.com/test/deep/about.html#yaya

What is actually happening?

Url became https://domain.com/test%2Fdeep%2Fabout.html#yaya

image


This is for the front-end of a CMS-supported project, where the front part of the URL is controlled by the CMS and it varies depending on the environment. Hence the need to first match it using :pathMatch() then match the actual page to render using :pageName().

Also, all pages has to be suffixed with .html, hence I am using a (kind of) complex regex to match the page. It works, however, I stumble upon this issue.

posva commented 1 year ago

This behavior is intended for regular params like :param because the / is a delimiter of params, but it should probably not happen for star params (:param(.*))

Note to myself in edit comment

keith0305 commented 1 year ago

Hey, thanks for the reply. Hopefully one day I can contribute a bug fix PR myself. For now, is there any workaround that you can suggest while I wait for the official fix?

keith0305 commented 1 year ago

Hi, is there any update to this? Perhaps point me to the correct place where this bug occurs so I can attempt to tackle the issue?

dschmidt commented 1 year ago

Here's a workaround that we/I've come up with - it's not perfect as it's applied to the whole path, so we enable it only for routes with meta.patchCleanPath === true but it works well enough for us for now (although we obviously would like to see an upstream fix, so we can get rid of this):

import { RouteLocation, RouteLocationNormalizedLoaded, RouteLocationRaw, Router } from 'vue-router'

export const patchRouter = (router: Router) => {
  const cleanPath = (route) =>
    [
      ['%2F', '/'],
      ['//', '/']
    ].reduce((path, rule) => path.replaceAll(rule[0], rule[1]), route || '')

  const bindResolve = router.resolve.bind(router)
  router.resolve = (
    raw: RouteLocationRaw,
    currentLocation?: RouteLocationNormalizedLoaded
  ): RouteLocation & {
    href: string
  } => {
    const resolved = bindResolve(raw, currentLocation)
    if (resolved.meta?.patchCleanPath !== true) {
      return resolved
    }

    return {
      ...resolved,
      href: cleanPath(resolved.href),
      path: cleanPath(resolved.path),
      fullPath: cleanPath(resolved.fullPath)
    }
  }

  const routerMethodFactory = (method) => (to) => {
    const resolved = router.resolve(to)
    if (resolved.meta?.patchCleanPath !== true) {
      return method(to)
    }

    return method({
      path: cleanPath(resolved.fullPath),
      query: resolved.query
    })
  }

  router.push = routerMethodFactory(router.push.bind(router))
  router.replace = routerMethodFactory(router.replace.bind(router))

  return router
}
keith0305 commented 1 year ago

Hey, thanks for your patch. Will try it out. It is very appreciated.

dschmidt commented 1 year ago

If you find any edge cases that don't work, I'm happy to hear.

keith0305 commented 1 year ago

Your patch works. Good enough for us to ship the site for now. Thank you so much @dschmidt.

memcorrupt commented 11 months ago

@posva Is there any plans to fix this behavior? I think that bef97d607d2edd77e2af8028938dc9a47bdf25fe is what caused the regression, since I used to be able to do things like redirecting to error pages without mangling the URL.

Tofandel commented 1 month ago

Same issue, it seems it doesn't happen when you use something like :category(.*)* but the problem is the catch all is greedy and causes issue because then the following /:category(.*?)*/(page/:page(\d+))? will never have page matched, it will go into category (which I think is also a behavior that should be changed or allow a second modifier token in the parser (like :category(.*?)*?to indicate non greediness)