From: Eduardo San Martin Morote <posva13@gmail.com>
Date: Fri, 17 Jan 2020 17:58:19 +0000 (+0100)
Subject: feat(matcher): handle param encoding
X-Git-Tag: v4.0.0-alpha.0~104
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=afcdb459d243e34be1f9ac9969d84dd2c7bc8f92;p=thirdparty%2Fvuejs%2Frouter.git

feat(matcher): handle param encoding
---

diff --git a/__tests__/matcher/resolve.spec.ts b/__tests__/matcher/resolve.spec.ts
index 210dcabb..2e152cfa 100644
--- a/__tests__/matcher/resolve.spec.ts
+++ b/__tests__/matcher/resolve.spec.ts
@@ -24,7 +24,12 @@ describe('Router Matcher', () => {
       start: MatcherLocationNormalized = START_LOCATION_NORMALIZED
     ) {
       record = Array.isArray(record) ? record : [record]
-      const matcher = createRouterMatcher(record)
+      const matcher = createRouterMatcher(
+        record,
+        {},
+        v => v,
+        v => v
+      )
 
       if (!('meta' in resolved)) {
         resolved.meta = record[0].meta || {}
@@ -382,7 +387,12 @@ describe('Router Matcher', () => {
           expected: MatcherLocationNormalized | MatcherLocationRedirect,
           currentLocation: MatcherLocationNormalized = START_LOCATION_NORMALIZED
         ) {
-          const matcher = createRouterMatcher(records)
+          const matcher = createRouterMatcher(
+            records,
+            {},
+            v => v,
+            v => v
+          )
           const resolved = matcher.resolve(location, currentLocation)
           expect(resolved).toEqual(expected)
           return resolved
diff --git a/__tests__/url-encoding.spec.ts b/__tests__/url-encoding.spec.ts
index 7dffd27c..49225063 100644
--- a/__tests__/url-encoding.spec.ts
+++ b/__tests__/url-encoding.spec.ts
@@ -1,13 +1,17 @@
-import { createRouter } from '../src/router'
+import { createRouter as newRouter } from '../src/router'
 import { createDom, components } from './utils'
 import { RouteRecord } from '../src/types'
 import { createMemoryHistory } from '../src'
+import * as encoding from '../src/utils/encoding'
+
+jest.mock('../src/utils/encoding')
 
 const routes: RouteRecord[] = [
   { path: '/', name: 'home', component: components.Home },
   { path: '/%25', name: 'percent', component: components.Home },
   { path: '/to-p/:p', redirect: to => `/p/${to.params.p}` },
   { path: '/p/:p', component: components.Bar, name: 'params' },
+  { path: '/p/:p+', component: components.Bar, name: 'repeat' },
 ]
 
 // this function is meant to easy refactor in the future as Histories are going to be
@@ -17,153 +21,74 @@ function createHistory() {
   return routerHistory
 }
 
+function createRouter() {
+  const history = createHistory()
+  const router = newRouter({ history, routes })
+  return router
+}
+
 // TODO: test by spying on encode functions since things are already tested by encoding.spec.ts
-describe.skip('URL Encoding', () => {
+describe('URL Encoding', () => {
   beforeAll(() => {
     createDom()
   })
 
-  describe('initial navigation', () => {
-    it('decodes path', async () => {
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.replace('/%25')
-      const { currentRoute } = router
-      expect(currentRoute.fullPath).toBe('/%25')
-      expect(currentRoute.path).toBe('/%25')
-    })
-
-    it('decodes params in path', async () => {
-      // /p/€
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/p/%E2%82%AC')
-      const { currentRoute } = router
-      expect(currentRoute.fullPath).toBe(encodeURI('/p/€'))
-      expect(currentRoute.path).toBe(encodeURI('/p/€'))
-      expect(currentRoute.params).toEqual({ p: '€' })
-    })
-
-    it('allows navigating to valid unencoded params (IE and Edge)', async () => {
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/p/€')
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('params')
-      // unfortunately, we cannot encode the path as we cannot know if it already encoded
-      // so comparing fullPath and path here is pointless
-      // fullPath: '/p/€',
-      // only the params matter
-      expect(currentRoute.params).toEqual({ p: '€' })
-    })
-
-    it('allows navigating to invalid unencoded params (IE and Edge)', async () => {
-      const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/p/%notvalid')
-      expect(spy).toHaveBeenCalledTimes(1)
-      spy.mockRestore()
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('params')
-      expect(currentRoute.params).toEqual({ p: '%notvalid' })
-    })
-
-    it('decodes params in query', async () => {
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/?q=%25%E2%82%AC')
-      expect(router.currentRoute).toEqual(
-        expect.objectContaining({
-          name: 'home',
-          fullPath: '/?q=' + encodeURIComponent('%€'),
-          query: {
-            q: '%€',
-          },
-          path: '/',
-        })
-      )
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('home')
-      expect(currentRoute.path).toBe('/')
-      expect(currentRoute.fullPath).toBe('/?q=' + encodeURIComponent('%€'))
-      expect(currentRoute.query).toEqual({ q: '%€' })
-    })
+  beforeEach(() => {
+    // mock all encoding functions
+    for (const key in encoding) {
+      // @ts-ignore
+      const value = encoding[key]
+      // @ts-ignore
+      if (typeof value === 'function') encoding[key] = jest.fn((v: string) => v)
+    }
+  })
 
-    it('decodes params keys in query', async () => {
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/?%E2%82%AC=euro')
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('home')
-      expect(currentRoute.path).toBe('/')
-      expect(currentRoute.fullPath).toBe(
-        '/?' + encodeURIComponent('€') + '=euro'
-      )
-      expect(currentRoute.query).toEqual({
-        query: {
-          '€': 'euro',
-        },
-      })
-    })
+  it('calls encodeParam with params object', async () => {
+    const router = createRouter()
+    await router.push({ name: 'params', params: { p: 'foo' } })
+    expect(encoding.encodeParam).toHaveBeenCalledTimes(1)
+    expect(encoding.encodeParam).toHaveBeenCalledWith('foo')
+  })
 
-    it('allow unencoded params in query (IE Edge)', async () => {
-      const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/?q=€%notvalid')
-      expect(spy).toHaveBeenCalledTimes(1)
-      spy.mockRestore()
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('home')
-      expect(currentRoute.path).toBe('/')
-      expect(currentRoute.fullPath).toBe(
-        '/?q=' + encodeURIComponent('€%notvalid')
-      )
-      expect(currentRoute.query).toEqual({
-        query: {
-          q: '€%notvalid',
-        },
-      })
-    })
+  it('calls encodeParam with relative location', async () => {
+    const router = createRouter()
+    await router.push('/p/bar')
+    await router.push({ params: { p: 'foo' } })
+    expect(encoding.encodeParam).toHaveBeenCalledTimes(1)
+    expect(encoding.encodeParam).toHaveBeenCalledWith('foo')
+  })
 
-    // TODO: we don't do this in current version of vue-router
-    // should we do it? it seems to be a bit different as it allows using % without
-    // encoding it. To be safe we would have to encode everything
-    it.skip('decodes hash', async () => {
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/#%25%E2%82%AC')
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('home')
-      expect(currentRoute.path).toBe('/')
-      expect(currentRoute.fullPath).toBe('/#' + encodeURIComponent('%€'))
-      expect(currentRoute.hash).toBe('#%€')
-    })
+  it('calls encodeParam with params object with arrays', async () => {
+    const router = createRouter()
+    await router.push({ name: 'repeat', params: { p: ['foo', 'bar'] } })
+    expect(encoding.encodeParam).toHaveBeenCalledTimes(2)
+    expect(encoding.encodeParam).toHaveBeenNthCalledWith(1, 'foo', 0, [
+      'foo',
+      'bar',
+    ])
+    expect(encoding.encodeParam).toHaveBeenNthCalledWith(2, 'bar', 1, [
+      'foo',
+      'bar',
+    ])
+  })
 
-    it('allow unencoded params in query (IE Edge)', async () => {
-      const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
-      const history = createHistory()
-      const router = createRouter({ history, routes })
-      await router.push('/?q=€%notvalid')
-      expect(spy).toHaveBeenCalledTimes(1)
-      spy.mockRestore()
-      const { currentRoute } = router
-      expect(currentRoute.name).toBe('home')
-      expect(currentRoute.path).toBe('/')
-      expect(currentRoute.fullPath).toBe(
-        '/?q=' + encodeURIComponent('€%notvalid')
-      )
-      expect(currentRoute.query).toEqual({
-        q: '€%notvalid',
-      })
-    })
+  it('calls decodeParam with a path', async () => {
+    const router = createRouter()
+    await router.push({ name: 'repeat', params: { p: ['foo', 'bar'] } })
+    expect(encoding.encodeParam).toHaveBeenCalledTimes(2)
+    expect(encoding.encodeParam).toHaveBeenNthCalledWith(1, 'foo', 0, [
+      'foo',
+      'bar',
+    ])
+    expect(encoding.encodeParam).toHaveBeenNthCalledWith(2, 'bar', 1, [
+      'foo',
+      'bar',
+    ])
   })
 
-  describe('resolving locations', () => {
+  describe.skip('resolving locations', () => {
     it('encodes params when resolving', async () => {
-      const history = createHistory()
-      const router = createRouter({ history, routes })
+      const router = createRouter()
       await router.push({ name: 'params', params: { p: '%€' } })
       const { currentRoute } = router
       expect(currentRoute.path).toBe(encodeURI('/p/%€'))
diff --git a/explorations/html5.html b/explorations/html5.html
index 8f06ef53..3f4118c6 100644
--- a/explorations/html5.html
+++ b/explorations/html5.html
@@ -95,13 +95,13 @@
           >
         </li>
         <li>
-          <router-link to="/n/€">/n/€</router-link>
+          <router-link to="/documents/€">/n/€</router-link>
         </li>
         <li>
-          <a href="/n/%E2%82%AC">/n/%E2%82%AC (force reload)</a>
+          <a href="/documents/%E2%82%AC">/documents/%E2%82%AC (force reload)</a>
         </li>
         <li>
-          <a href="/n/€">/n/€ (force reload): not valid tho</a>
+          <a href="/documents/€">/documents/€ (force reload): not valid tho</a>
         </li>
         <li>
           <router-link to="/">/</router-link>
diff --git a/src/history/common.ts b/src/history/common.ts
index 8149060a..7b2d31eb 100644
--- a/src/history/common.ts
+++ b/src/history/common.ts
@@ -1,5 +1,5 @@
 import { ListenerRemover } from '../types'
-import { encodeQueryProperty, encodeHash, encodePath } from '../utils/encoding'
+import { encodeQueryProperty, encodeHash } from '../utils/encoding'
 
 export type HistoryQuery = Record<string, string | string[]>
 // TODO: is it reall worth allowing null to form queries like ?q&b&c
@@ -142,31 +142,31 @@ export function parseURL(location: string): HistoryLocationNormalized {
 // TODO: the encoding would be handled at a router level instead where encoding functions can be customized
 // that way the matcher can encode/decode params properly
 
-function safeDecodeUriComponent(value: string): string {
-  try {
-    value = decodeURIComponent(value)
-  } catch (err) {
-    // TODO: handling only URIError?
-    console.warn(
-      `[vue-router] error decoding query "${value}". Keeping the original value.`
-    )
-  }
-
-  return value
-}
-
-function safeEncodeUriComponent(value: string): string {
-  try {
-    value = encodeURIComponent(value)
-  } catch (err) {
-    // TODO: handling only URIError?
-    console.warn(
-      `[vue-router] error encoding query "${value}". Keeping the original value.`
-    )
-  }
-
-  return value
-}
+// function safeDecodeUriComponent(value: string): string {
+//   try {
+//     value = decodeURIComponent(value)
+//   } catch (err) {
+//     // TODO: handling only URIError?
+//     console.warn(
+//       `[vue-router] error decoding query "${value}". Keeping the original value.`
+//     )
+//   }
+
+//   return value
+// }
+
+// function safeEncodeUriComponent(value: string): string {
+//   try {
+//     value = encodeURIComponent(value)
+//   } catch (err) {
+//     // TODO: handling only URIError?
+//     console.warn(
+//       `[vue-router] error encoding query "${value}". Keeping the original value.`
+//     )
+//   }
+
+//   return value
+// }
 
 /**
  * Transform a queryString into a query object. Accept both, a version with the leading `?` and without
@@ -205,9 +205,7 @@ export function stringifyURL(location: HistoryLocation): string {
   let url = location.path
   let query = location.query ? stringifyQuery(location.query) : ''
 
-  return (
-    encodePath(url) + (query && '?' + query) + encodeHash(location.hash || '')
-  )
+  return url + (query && '?' + query) + encodeHash(location.hash || '')
 }
 
 /**
diff --git a/src/matcher/index.ts b/src/matcher/index.ts
index ab93e1e2..a357cdda 100644
--- a/src/matcher/index.ts
+++ b/src/matcher/index.ts
@@ -30,9 +30,26 @@ function removeTrailingSlash(path: string): string {
   return path.replace(TRAILING_SLASH_RE, '$1')
 }
 
+function applyToParam(
+  fn: (v: string) => string,
+  params: PathParams
+): PathParams {
+  const newParams: PathParams = {}
+
+  // TODO: could also normalize values like numbers and stuff
+  for (const key in params) {
+    const value = params[key]
+    newParams[key] = Array.isArray(value) ? value.map(fn) : fn(value)
+  }
+
+  return newParams
+}
+
 export function createRouterMatcher(
   routes: RouteRecord[],
-  globalOptions?: PathParserOptions
+  globalOptions: PathParserOptions,
+  encodeParam: (param: string) => string,
+  decodeParam: (param: string) => string
 ): RouterMatcher {
   const matchers: RouteRecordMatcher[] = []
 
@@ -129,11 +146,11 @@ export function createRouterMatcher(
       if (!matcher) throw new NoRouteMatchError(location)
 
       name = matcher.record.name
-      // TODO: merge params
+      // TODO: merge params with current location. Should this be done by name. I think there should be some kind of relationship between the records like children of a parent should keep parent props but not the rest
       params = location.params || currentLocation.params
       // params are automatically encoded
       // TODO: try catch to provide better error messages
-      path = matcher.stringify(params)
+      path = matcher.stringify(applyToParam(encodeParam, params))
 
       if ('redirect' in matcher.record) {
         const { redirect } = matcher.record
@@ -157,7 +174,7 @@ export function createRouterMatcher(
       // TODO: warning of unused params if provided
       if (!matcher) throw new NoRouteMatchError(location)
 
-      params = matcher.parse(location.path)!
+      params = applyToParam(decodeParam, matcher.parse(location.path)!)
       // no need to resolve the path with the matcher as it was provided
       // this also allows the user to control the encoding
       // TODO: check if the note above regarding encoding is still true
@@ -187,7 +204,7 @@ export function createRouterMatcher(
       if (!matcher) throw new NoRouteMatchError(location, currentLocation)
       name = matcher.record.name
       params = location.params || currentLocation.params
-      path = matcher.stringify(params)
+      path = matcher.stringify(applyToParam(encodeParam, params))
     }
 
     // this should never happen because it will mean that the user ended up in a route
diff --git a/src/router.ts b/src/router.ts
index 3518dd05..7f55604f 100644
--- a/src/router.ts
+++ b/src/router.ts
@@ -32,6 +32,8 @@ import {
 } from './errors'
 import { extractComponentsGuards, guardToPromiseFn } from './utils'
 import Vue from 'vue'
+import { encodeParam } from './utils/encoding'
+import { decode } from './utils/encoding'
 
 type ErrorHandler = (error: any) => any
 // resolve, reject arguments of Promise constructor
@@ -78,7 +80,10 @@ export function createRouter({
   scrollBehavior,
 }: RouterOptions): Router {
   const matcher: ReturnType<typeof createRouterMatcher> = createRouterMatcher(
-    routes
+    routes,
+    {},
+    encodeParam,
+    decode
   )
   const beforeGuards: NavigationGuard[] = []
   const afterGuards: PostNavigationGuard[] = []