Dynamic Routes: Change impl from $param to [param] (#7623)

* Dynamic Routes: Change impl from $param to [param]

* Update expected test snapshot

* Update test to use new syntax

* Update test file

* Test more behavior

* Update route sorter for new param syntax

* Update dynamic routing tests

* Update danging test file

* Tweak test

* Fix dev and update tests

packages/next-server/lib/router/utils/is-dynamic.ts

+// Identify /[param]/ in route string
+const TEST_ROUTE = /\/\[[^\/]+?\](?=\/|$)/
+
 export function isDynamicRoute(route: string): boolean {
-  return route.indexOf('/$') !== -1
+  return TEST_ROUTE.test(route)
 }

packages/next-server/lib/router/utils/route-regex.ts

 export function getRouteRegex(
   normalizedRoute: string
 ): { re: RegExp; groups: { [groupName: string]: number } } {
+  // Escape all characters that could be considered RegEx
   const escapedRoute = (normalizedRoute.replace(/\/$/, '') || '/').replace(
     /[|\\{}()[\]^$+*?.-]/g,
     '\\$&'
@@ -10,20 +11,13 @@ export function getRouteRegex(
   let groupIndex = 1
 
   const parameterizedRoute = escapedRoute.replace(
-    /\/\\\$([^\/]+?)(?=\/|$)/g,
+    /\/\\\[([^\/]+?)\\\](?=\/|$)/g,
     (_, $1) => (
       (groups[
         $1
-          // // Remove optional parameter marker
-          // .replace(/\\\$$/, '')
           // Un-escape key
           .replace(/\\([|\\{}()[\]^$+*?.-])/g, '$1')
       ] = groupIndex++),
-      // // Test the route for an optional parameter
-      // $1.lastIndexOf('$') === $1.length - 1
-      //   ? // Optional: match a param
-      //     '(?:/([^/]+?))?'
-      //   : // Required: match a param
       '/([^/]+?)'
     )
   )

packages/next-server/lib/router/utils/sorted-routes.ts

@@ -18,7 +18,7 @@ class UrlNode {
   private _smoosh(prefix: string = '/'): string[] {
     const childrenPaths = [...this.children.keys()].sort()
     if (this.hasSlug) {
-      childrenPaths.splice(childrenPaths.indexOf('$'), 1)
+      childrenPaths.splice(childrenPaths.indexOf('[]'), 1)
     }
 
     const routes = childrenPaths
@@ -27,7 +27,7 @@ class UrlNode {
 
     if (this.hasSlug) {
       routes.push(
-        ...this.children.get('$')!._smoosh(`${prefix}$${this.slugName}/`)
+        ...this.children.get('[]')!._smoosh(`${prefix}[${this.slugName}]/`)
       )
     }
 
@@ -45,8 +45,8 @@ class UrlNode {
     }
 
     let [nextSegment] = urlPaths
-    if (nextSegment.startsWith('$')) {
-      const slugName = nextSegment.substring(1)
+    if (nextSegment.startsWith('[') && nextSegment.endsWith(']')) {
+      const slugName = nextSegment.slice(1, -1)
       if (this.hasSlug && slugName !== this.slugName) {
         throw new Error(
           'You cannot use different slug names for the same dynamic path.'
@@ -54,7 +54,7 @@ class UrlNode {
       }
 
       this.slugName = slugName
-      nextSegment = '$'
+      nextSegment = '[]'
     }
 
     if (!this.children.has(nextSegment)) {

packages/next/client/index.js

@@ -81,7 +81,7 @@ class Container extends React.Component {
     this.scrollToHash()
 
     // If page was exported and has a querystring
-    // If it's a dynamic route (/$ inside) or has a querystring
+    // If it's a dynamic route or has a querystring
     if (
       data.nextExport &&
       (isDynamicRoute(router.pathname) || window.location.search)

packages/next/server/next-dev-server.js

@@ -114,14 +114,16 @@ export default class DevServer extends Server {
           }
 
           let pageName = '/' + relative(pagesDir, fileName).replace(/\\+/g, '/')
-          if (!isDynamicRoute(pageName)) {
-            continue
-          }
 
           const pageExt = extname(pageName)
           pageName = pageName.slice(0, -pageExt.length)
 
-          pageName = pageName.replace(/\/index$/, '')
+          pageName = pageName.replace(/\/index$/, '') || '/'
+
+          if (!isDynamicRoute(pageName)) {
+            continue
+          }
+
           dynamicRoutedPages.push(pageName)
         }
 

test/integration/dynamic-routing/pages/index.js

@@ -3,27 +3,23 @@ import Link from 'next/link'
 const Page = () => (
   <div>
     <h3>My blog</h3>
-    <Link href='/$post' as='/post-1'>
+    <Link href='/[post]' as='/post-1'>
       <a id='view-post-1'>View post 1</a>
     </Link>
     <br />
-    <Link href='/$post/comments' as='/post-1/comments'>
+    <Link href='/[post]/comments' as='/post-1/comments'>
       <a id='view-post-1-comments'>View post 1 comments</a>
     </Link>
     <br />
-    <Link href='/$post/$comment' as='/post-1/comment-1'>
+    <Link href='/[post]/[comment]' as='/post-1/comment-1'>
       <a id='view-post-1-comment-1'>View comment 1 on post 1</a>
     </Link>
-    {/* <br />
-    <Link href='/blog/$post/comment/$id$' as='/blog/543/comment'>
-      <a id='view-blog-post-1-comments'>View all comments on blog post 543</a>
-    </Link> */}
     <br />
-    <Link href='/blog/$post/comment/$id' as='/blog/321/comment/123'>
+    <Link href='/blog/[post]/comment/[id]' as='/blog/321/comment/123'>
       <a id='view-nested-dynamic-cmnt'>View comment 123 on blog post 321</a>
     </Link>
     <br />
-    <Link href='/$post?fromHome=true' as='/post-1?fromHome=true'>
+    <Link href='/[post]?fromHome=true' as='/post-1?fromHome=true'>
       <a id='view-post-1-with-query'>View post 1 with query</a>
     </Link>
   </div>

test/integration/dynamic-routing/pages/on-mount/$post.js → test/integration/dynamic-routing/pages/on-mount/[post].js

@@ -3,5 +3,5 @@ import { useRouter } from 'next/router'
 export default () => {
   const router = useRouter()
   const { query } = router
-  return <p>post: {query.post}</p>
+  return <p>onmpost: {query.post || 'pending'}</p>
 }

test/integration/dynamic-routing/test/index.test.js

@@ -142,10 +142,13 @@ function runTests () {
   })
 
   it('should update dynamic values on mount', async () => {
+    const html = await renderViaHTTP(appPort, '/on-mount/post-1')
+    expect(html).toMatch(/onmpost:.*pending/)
+
     const browser = await webdriver(appPort, '/on-mount/post-1')
     await waitFor(1000)
     const text = await browser.eval(`document.body.innerHTML`)
-    expect(text).toMatch(/post:.*post-1/)
+    expect(text).toMatch(/onmpost:.*post-1/)
   })
 }
 

test/unit/__snapshots__/page-route-sorter.test.js.snap

@@ -3,12 +3,12 @@
 exports[`getSortedRoutes correctly sorts required slugs 1`] = `
 Array [
   "/",
-  "/apples/$ab/$cd/ef",
-  "/blog/$id",
-  "/blog/$id/comments/$cid",
-  "/foo/$d/bar/baz/$f",
+  "/apples/[ab]/[cd]/ef",
+  "/blog/[id]",
+  "/blog/[id]/comments/[cid]",
+  "/foo/[d]/bar/baz/[f]",
   "/posts",
-  "/posts/$id",
-  "/$root-slug",
+  "/posts/[id]",
+  "/[root-slug]",
 ]
 `;

test/unit/page-route-sorter.test.js

@@ -5,14 +5,14 @@ describe('getSortedRoutes', () => {
   it('does not add extra routes', () => {
     expect(getSortedRoutes(['/posts'])).toEqual(['/posts'])
 
-    expect(getSortedRoutes(['/posts/$id'])).toEqual(['/posts/$id'])
-    expect(getSortedRoutes(['/posts/$id/foo'])).toEqual(['/posts/$id/foo'])
+    expect(getSortedRoutes(['/posts/[id]'])).toEqual(['/posts/[id]'])
+    expect(getSortedRoutes(['/posts/[id]/foo'])).toEqual(['/posts/[id]/foo'])
 
-    expect(getSortedRoutes(['/posts/$id/$foo/bar'])).toEqual([
-      '/posts/$id/$foo/bar'
+    expect(getSortedRoutes(['/posts/[id]/[foo]/bar'])).toEqual([
+      '/posts/[id]/[foo]/bar'
     ])
-    expect(getSortedRoutes(['/posts/$id/baz/$foo/bar'])).toEqual([
-      '/posts/$id/baz/$foo/bar'
+    expect(getSortedRoutes(['/posts/[id]/baz/[foo]/bar'])).toEqual([
+      '/posts/[id]/baz/[foo]/bar'
     ])
   })
 
@@ -20,14 +20,26 @@ describe('getSortedRoutes', () => {
     expect(
       getSortedRoutes([
         '/posts',
-        '/$root-slug',
+        '/[root-slug]',
         '/',
-        '/posts/$id',
-        '/blog/$id/comments/$cid',
-        '/blog/$id',
-        '/foo/$d/bar/baz/$f',
-        '/apples/$ab/$cd/ef'
+        '/posts/[id]',
+        '/blog/[id]/comments/[cid]',
+        '/blog/[id]',
+        '/foo/[d]/bar/baz/[f]',
+        '/apples/[ab]/[cd]/ef'
       ])
     ).toMatchSnapshot()
   })
+
+  it('catches mismatched param names', () => {
+    expect(() =>
+      getSortedRoutes([
+        '/',
+        '/blog',
+        '/blog/[id]',
+        '/blog/[id]/comments/[cid]',
+        '/blog/[cid]'
+      ])
+    ).toThrowError(/different slug names/)
+  })
 })