Discard optional paren history based on params evaluation (remix-run#…

…3782)

* Discard paren history based on params evaluation

* Keeping track of parens and nesting parens

* Specific variable for paren history; removing debugging invariant; fixing findIndex IE10 issue

* Fixing invariant messages and writing tests for invariants

modules/PatternUtils.js

@@ -149,7 +149,7 @@ export function formatPattern(pattern, params) {
   params = params || {}
 
   const { tokens } = compilePattern(pattern)
-  let parenCount = 0, pathname = '', splatIndex = 0
+  let parenCount = 0, pathname = '', splatIndex = 0, parenHistory = []
 
   let token, paramName, paramValue
   for (let i = 0, len = tokens.length; i < len; ++i) {
@@ -167,9 +167,16 @@ export function formatPattern(pattern, params) {
       if (paramValue != null)
         pathname += encodeURI(paramValue)
     } else if (token === '(') {
+      parenHistory[parenCount] = ''
       parenCount += 1
     } else if (token === ')') {
+      const parenText = parenHistory.pop()
       parenCount -= 1
+
+      if (parenCount)
+        parenHistory[parenCount - 1] += parenText
+      else
+        pathname += parenText
     } else if (token.charAt(0) === ':') {
       paramName = token.substring(1)
       paramValue = params[paramName]
@@ -180,12 +187,47 @@ export function formatPattern(pattern, params) {
         paramName, pattern
       )
 
-      if (paramValue != null)
+      if (paramValue == null) {
+        if (parenCount) {
+          parenHistory[parenCount - 1] = ''
+
+          const curTokenIdx = tokens.indexOf(token)
+          const tokensSubset = tokens.slice(curTokenIdx, tokens.length)
+          let nextParenIdx = -1
+
+          for (let i = 0; i < tokensSubset.length; i++) {
+            if (tokensSubset[i] == ')') {
+              nextParenIdx = i
+              break
+            }
+          }
+
+          invariant(
+            nextParenIdx > 0,
+            'Path "%s" is missing end paren at segment "%s"', pattern, tokensSubset.join('')
+          )
+
+          // jump to ending paren
+          i = curTokenIdx + nextParenIdx - 1
+        }
+      }
+      else if (parenCount)
+        parenHistory[parenCount - 1] += encodeURIComponent(paramValue)
+      else
         pathname += encodeURIComponent(paramValue)
+
     } else {
-      pathname += token
+      if (parenCount)
+        parenHistory[parenCount - 1] += token
+      else
+        pathname += token
     }
   }
 
+  invariant(
+    parenCount <= 0,
+    'Path "%s" is missing end paren', pattern
+  )
+
   return pathname.replace(/\/+/g, '/')
 }

modules/__tests__/formatPattern-test.js

@@ -10,6 +10,28 @@ describe('formatPattern', function () {
     })
   })
 
+  describe('with a bad pattern', function () {
+    describe('that is missing an end param', function () {
+      const pattern = '/comments/(:id'
+
+      describe('with no value given to the formatPattern function', function () {
+        it('throws an error', function () {
+          expect(function () {
+            formatPattern(pattern, {})
+          }).toThrow(/Path "\/comments\/\(:id" is missing end paren at segment ":id"/)
+        })
+      })
+
+      describe('with value given to the formatPattern function', function () {
+        it('throws an error', function () {
+          expect(function () {
+            formatPattern(pattern, { id: '1' })
+          }).toThrow(/Path "\/comments\/\(:id" is missing end paren/)
+        })
+      })
+    })
+  })
+
   describe('when a pattern has dynamic segments', function () {
     const pattern = '/comments/:id/edit'
 
@@ -45,6 +67,98 @@ describe('formatPattern', function () {
       })
     })
 
+    describe('and a param is optional with addtional text prior to the params', function () {
+      const pattern = '/search(/forum/:id)'
+
+      it('returns the correct path when param is supplied', function () {
+        expect(formatPattern(pattern, { id:'123' })).toEqual('/search/forum/123')
+      })
+
+      it('returns the correct path when param is not supplied', function () {
+        expect(formatPattern(pattern, { })).toEqual('/search')
+      })
+    })
+
+    describe('and a param is optional with multiple segments and addtional text prior to the params', function () {
+      const pattern = '/search(/forum/:forum_id)(/comment/:comment_id)'
+
+      it('returns the correct path when param is supplied', function () {
+        expect(formatPattern(pattern, { forum_id: '123', comment_id: '456' })).toEqual('/search/forum/123/comment/456')
+      })
+
+      it('returns the correct path when forum_id param is supplied', function () {
+        expect(formatPattern(pattern, { forum_id: '123' })).toEqual('/search/forum/123')
+      })
+
+      it('returns the correct path when comment_id param is supplied', function () {
+        expect(formatPattern(pattern, { comment_id: '456' })).toEqual('/search/comment/456')
+      })
+
+      it('returns the correct path when param is not supplied', function () {
+        expect(formatPattern(pattern, { })).toEqual('/search')
+      })
+    })
+
+    describe('and a param is optional with multiple segments in the optional part', function () {
+      const pattern = '/search(/forum/:forum_id/comment/:comment_id)'
+
+      it('returns the correct path when param is supplied', function () {
+        expect(formatPattern(pattern, { forum_id:'123', comment_id: '456' })).toEqual('/search/forum/123/comment/456')
+      })
+
+      it('returns the correct path when forum_id param is not supplied', function () {
+        expect(formatPattern(pattern, { forum_id:'123' })).toEqual('/search')
+      })
+
+      it('returns the correct path when comment_id param is not supplied 2', function () {
+        expect(formatPattern(pattern, { comment_id:'456' })).toEqual('/search')
+      })
+
+      it('returns the correct path when param is not supplied', function () {
+        expect(formatPattern(pattern, { })).toEqual('/search')
+      })
+    })
+
+    describe('and a param is optional with nested optional segments with addtional text prior to the params', function () {
+      const pattern = '/search(/forum/:forum_id(/comment/:comment_id))'
+
+      it('returns the correct path when params are supplied', function () {
+        expect(formatPattern(pattern, { forum_id:'123', comment_id: '456' })).toEqual('/search/forum/123/comment/456')
+      })
+
+      it('returns the correct path when forum_id param is supplied', function () {
+        expect(formatPattern(pattern, { forum_id:'123' })).toEqual('/search/forum/123')
+      })
+
+      it('returns the correct path when comment_id param is supplied', function () {
+        expect(formatPattern(pattern, { comment_id: '456' })).toEqual('/search')
+      })
+
+      it('returns the correct path when param is not supplied', function () {
+        expect(formatPattern(pattern, { })).toEqual('/search')
+      })
+    })
+
+    describe('and a param is optional with nested optional segments with addtional text prior to the params in different order', function () {
+      const pattern = '/search((/forum/:forum_id)/comment/:comment_id)'
+
+      it('returns the correct path when params are supplied', function () {
+        expect(formatPattern(pattern, { forum_id:'123', comment_id: '456' })).toEqual('/search/forum/123/comment/456')
+      })
+
+      it('returns the correct path when comment_id param is supplied', function () {
+        expect(formatPattern(pattern, { comment_id: '456' })).toEqual('/search/comment/456')
+      })
+
+      it('returns the correct path when forum_id param is supplied', function () {
+        expect(formatPattern(pattern, { forum_id:'123' })).toEqual('/search')
+      })
+
+      it('returns the correct path when param is not supplied', function () {
+        expect(formatPattern(pattern, { })).toEqual('/search')
+      })
+    })
+
     describe('and all params are present', function () {
       it('returns the correct path', function () {
         expect(formatPattern(pattern, { id: 'abc' })).toEqual('/comments/abc/edit')