Repository: delvedor/find-my-way
Pull request #182 (2021-03-09 05:51:39)

Pretty print bugfix and revision
I found some unexpected behaviour in the way routes were displayed while I was testing this feature addition for `fastify-autoload` (https://github.com/fastify/fastify-autoload/pull/134)

The route parameters weren't being properly placed when drawing the tree if there was no root item. An example can be found in the PR linked above. I added a test for a specific instance where I was able to get it to break.

I have also added an additional tree-drawing function which uses the `findMyWay.routes` array instead of the radix tree, as it might be more intuitive to some users because it doesn't break trees by common prefixes (e.g. `home, homer, honour` in place of `ho->me, ho->me->r, ho->nour`)

If the added function is appropriate, I will add some tests for it.

mcollina: I'm ok with the added function.
polymathca: Two more notes:

I had to modify the constrained parametric route test, as it wasn't including the preceding slashes in the expected output on the constrained paths. I don't think that should be problematic, but it's worth mentioning. Link to that change here:

https://github.com/polymathca/find-my-way/blob/4a92e9af8fc6f0e091a75aa1ef0e3e0ef886db9a/test/pretty-print.test.js#L124-L129

The `routesArray: true` output explicitly indicates nodes without methods attached with a `(-)` which I feel is a more explicit way to show no handler is attached at that level. I can revise that if it's not correct (I left that as-is in the original function).
polymathca: > 
> 
> Great work! Can you flip the way the option works?
> `commonPrefix: false` shows the full name of each path chunk
> `commonPrefix: true` shows how the prefix is split internally (which is the default)

For sure! Or do you think it would be better to rename the option (e.g. `useFullPaths`) so we aren't altering the current default behaviour of showing the radix tree when the function is called without options?
delvedor: `commonPrefix` is fine, the default behavior is `commonPrefix:true`
polymathca: Sounds good, I added a test that explicitly shows the behaviour of `commonPrefix: true` and `commonPrefix: false`, just for clarity
polymathca: I tidied up the branch-drawing somewhat so the `commonPrefix: true` output doesn't insert spaces in new items with constraints, and the `commonPrefix: false` version now outputs constrained routes on separate lines for clarity.

Patch:
diff --git a/README.md b/README.md
index b7b22e9..54fd5ec 100644
--- a/README.md
+++ b/README.md
@@ -409,18 +409,34 @@ router.find('GET', '/example', { host: 'fastify.io', version: '1.x' })
 ```
 
 <a name="pretty-print"></a>
-#### prettyPrint()
+#### prettyPrint([{ commonPrefix: false }])
 Prints the representation of the internal radix tree, useful for debugging.
 ```js
 findMyWay.on('GET', '/test', () => {})
 findMyWay.on('GET', '/test/hello', () => {})
-findMyWay.on('GET', '/hello/world', () => {})
+findMyWay.on('GET', '/testing', () => {})
+findMyWay.on('GET', '/testing/:param', () => {})
+findMyWay.on('PUT', '/update', () => {})
 
 console.log(findMyWay.prettyPrint())
 // └── /
-//   ├── test (GET)
-//   │   └── /hello (GET)
-//   └── hello/world (GET)
+//     ├── test (GET)
+//     │   ├── /hello (GET)
+//     │   └── ing (GET)
+//     │       └── /:param (GET)
+//     └── update (PUT)
+```
+
+`prettyPrint` accepts an optional setting to use the internal routes array to render the tree.
+
+```js
+console.log(findMyWay.prettyPrint({ commonPrefix: false }))
+// └── / (-)
+//     ├── test (GET)
+//     │   └── /hello (GET)
+//     ├── testing (GET)
+//     │   └── /:param (GET)
+//     └── update (PUT)
 ```
 
 <a name="routes"></a>
diff --git a/index.js b/index.js
index ee3f1ab..6bdb960 100644
--- a/index.js
+++ b/index.js
@@ -15,7 +15,7 @@ const assert = require('assert')
 const http = require('http')
 const fastDecode = require('fast-decode-uri-component')
 const isRegexSafe = require('safe-regex2')
-const { flattenNode, compressFlattenedNode, prettyPrintFlattenedNode } = require('./lib/pretty-print')
+const { flattenNode, compressFlattenedNode, prettyPrintFlattenedNode, prettyPrintRoutesArray } = require('./lib/pretty-print')
 const Node = require('./node')
 const Constrainer = require('./lib/constrainer')
 
@@ -571,7 +571,9 @@ Router.prototype._onBadUrl = function (path) {
   }
 }
 
-Router.prototype.prettyPrint = function () {
+Router.prototype.prettyPrint = function (opts = {}) {
+  opts.commonPrefix = opts.commonPrefix === undefined ? true : opts.commonPrefix // default to original behaviour
+  if (!opts.commonPrefix) return prettyPrintRoutesArray(this.routes)
   const root = {
     prefix: '/',
     nodes: [],
diff --git a/lib/pretty-print.js b/lib/pretty-print.js
index 56f6e9c..11383cc 100644
--- a/lib/pretty-print.js
+++ b/lib/pretty-print.js
@@ -1,5 +1,153 @@
+'use strict'
+
+/* eslint-disable no-multi-spaces */
+const indent              = '    '
+const branchIndent        = '│   '
+const midBranchIndent     = '├── '
+const endBranchIndent     = '└── '
+const wildcardDelimiter   = '*'
+const pathDelimiter       = '/'
+const pathRegExp          = /(?=\/)/
+/* eslint-enable */
+
+function prettyPrintRoutesArray (routeArray) {
+  const mergedRouteArray = []
+
+  let tree = ''
+
+  routeArray.sort((a, b) => {
+    if (!a.path || !b.path) return 0
+    return a.path.localeCompare(b.path)
+  })
+
+  // merge alike paths
+  for (let i = 0; i < routeArray.length; i++) {
+    const route = routeArray[i]
+    const pathExists = mergedRouteArray.find(r => route.path === r.path)
+    if (pathExists) {
+      // path already declared, add new method and break out of loop
+      pathExists.handlers.push({
+        method: route.method,
+        opts: route.opts.constraints || undefined
+      })
+      continue
+    }
+
+    const routeHandler = {
+      method: route.method,
+      opts: route.opts.constraints || undefined
+    }
+    mergedRouteArray.push({
+      path: route.path,
+      methods: [route.method],
+      opts: [route.opts],
+      handlers: [routeHandler],
+      parents: [],
+      branchLevel: 1
+    })
+  }
+
+  // insert root level path if none defined
+  if (!mergedRouteArray.filter(r => r.path === pathDelimiter).length) {
+    const rootPath = {
+      path: pathDelimiter,
+      truncatedPath: '',
+      methods: [],
+      opts: [],
+      handlers: [{}],
+      parents: [pathDelimiter]
+    }
+
+    // if wildcard route exists, insert root level after wildcard
+    if (mergedRouteArray.filter(r => r.path === wildcardDelimiter).length) {
+      mergedRouteArray.splice(1, 0, rootPath)
+    } else {
+      mergedRouteArray.unshift(rootPath)
+    }
+  }
+
+  // build tree
+  const routeTree = buildRouteTree(mergedRouteArray)
+
+  // draw tree
+  routeTree.forEach((rootBranch, idx) => {
+    tree += drawBranch(rootBranch, null, idx === routeTree.length - 1, false, true)
+    tree += '\n' // newline characters inserted at beginning of drawing function to allow for nested paths
+  })
+
+  return tree
+}
+
+function buildRouteTree (mergedRouteArray, rootPath) {
+  rootPath = rootPath || pathDelimiter
+
+  const result = []
+  const temp = { result }
+  mergedRouteArray.forEach((route, idx) => {
+    let splitPath = route.path.split(pathRegExp)
+
+    // add preceding slash for proper nesting
+    if (splitPath[0] !== pathDelimiter) {
+      // handle wildcard route
+      if (splitPath[0] !== wildcardDelimiter) splitPath = [pathDelimiter, splitPath[0].slice(1), ...splitPath.slice(1)]
+    }
+
+    // build tree
+    splitPath.reduce((acc, path, pidx) => {
+      if (!acc[path]) {
+        acc[path] = { result: [] }
+        const pathSeg = { path, children: acc[path].result }
+
+        if (pidx === splitPath.length - 1) pathSeg.handlers = route.handlers
+        acc.result.push(pathSeg)
+      }
+      return acc[path]
+    }, temp)
+  })
+
+  // unfold root object from array
+  return result
+}
+
+function drawBranch (pathSeg, prefix, endBranch, noPrefix, rootBranch) {
+  let branch = ''
+
+  if (!noPrefix && !rootBranch) branch += '\n'
+  if (!noPrefix) branch += `${prefix || ''}${endBranch ? endBranchIndent : midBranchIndent}`
+  branch += `${pathSeg.path}`
+
+  if (pathSeg.handlers) {
+    const flatHandlers = pathSeg.handlers.reduce((acc, curr) => {
+      const match = acc.findIndex(h => JSON.stringify(h.opts) === JSON.stringify(curr.opts))
+      if (match !== -1) {
+        acc[match].method = [acc[match].method, curr.method].join(', ')
+      } else {
+        acc.push(curr)
+      }
+      return acc
+    }, [])
+
+    flatHandlers.forEach((handler, idx) => {
+      if (idx > 0) branch += `${noPrefix ? '' : prefix}${endBranch ? indent : branchIndent}${pathSeg.path}`
+      branch += ` (${handler.method || '-'})`
+      if (handler.opts && JSON.stringify(handler.opts) !== '{}') branch += ` ${JSON.stringify(handler.opts)}`
+      if (flatHandlers.length > 1 && idx !== flatHandlers.length - 1) branch += '\n'
+    })
+  }
+
+  if (!noPrefix) prefix = `${prefix || ''}${endBranch ? indent : branchIndent}`
+
+  pathSeg.children.forEach((child, idx) => {
+    const endBranch = idx === pathSeg.children.length - 1
+    const skipPrefix = (!pathSeg.handlers && pathSeg.children.length === 1)
+    branch += drawBranch(child, prefix, endBranch, skipPrefix)
+  })
+
+  return branch
+}
+
 function prettyPrintFlattenedNode (flattenedNode, prefix, tail) {
-  var paramName = ''
+  let paramName = ''
   const printHandlers = []
 
   for (const node of flattenedNode.nodes) {
@@ -8,39 +156,49 @@ function prettyPrintFlattenedNode (flattenedNode, prefix, tail) {
     }
   }
 
-  printHandlers.forEach((handler, index) => {
-    let suffix = `(${handler.method}`
-    if (Object.keys(handler.constraints).length > 0) {
-      suffix += ' ' + JSON.stringify(handler.constraints)
-    }
-    suffix += ')'
-
-    let name = ''
-    if (flattenedNode.prefix.includes(':')) {
-      var params = handler.params
-      name = params[params.length - 1]
-      if (index > 0) {
-        name = ':' + name
+  if (printHandlers.length) {
+    printHandlers.forEach((handler, index) => {
+      let suffix = `(${handler.method || '-'})`
+      if (Object.keys(handler.constraints).length > 0) {
+        suffix += ' ' + JSON.stringify(handler.constraints)
       }
-    } else if (index > 0) {
-      name = flattenedNode.prefix
-    }
 
-    if (index === 0) {
-      paramName += name + ` ${suffix}`
-      return
-    } else {
-      paramName += '\n'
-    }
+      let name = ''
+      // find locations of parameters in prefix
+      const paramIndices = flattenedNode.prefix.split('').map((ch, idx) => ch === ':' ? idx : null).filter(idx => idx !== null)
+      if (paramIndices.length) {
+        let prevLoc = 0
+        paramIndices.forEach((loc, idx) => {
+          // find parameter in prefix
+          name += flattenedNode.prefix.slice(prevLoc, loc + 1)
+          // insert parameters
+          name += handler.params[handler.params.length - paramIndices.length + idx]
+          if (idx === paramIndices.length - 1) name += flattenedNode.prefix.slice(loc + 1)
+          prevLoc = loc + 1
+        })
+      } else {
+        // there are no parameters, return full object
+        name = flattenedNode.prefix
+      }
 
-    paramName += prefix + '    ' + name + ` ${suffix}`
-  })
+      if (index === 0) {
+        paramName += `${name} ${suffix}`
+        return
+      } else {
+        paramName += '\n'
+      }
 
-  var tree = `${prefix}${tail ? '└── ' : '├── '}${flattenedNode.prefix}${paramName}\n`
+      paramName += `${prefix}${tail ? indent : branchIndent}${name} ${suffix}`
+    })
+  } else {
+    paramName = flattenedNode.prefix
+  }
+
+  let tree = `${prefix}${tail ? endBranchIndent : midBranchIndent}${paramName}\n`
 
-  prefix = `${prefix}${tail ? '    ' : '│   '}`
+  prefix = `${prefix}${tail ? indent : branchIndent}`
   const labels = Object.keys(flattenedNode.children)
-  for (var i = 0; i < labels.length; i++) {
+  for (let i = 0; i < labels.length; i++) {
     const child = flattenedNode.children[labels[i]]
     tree += prettyPrintFlattenedNode(child, prefix, i === (labels.length - 1))
   }
@@ -54,7 +212,8 @@ function flattenNode (flattened, node) {
 
   if (node.children) {
     for (const child of Object.values(node.children)) {
-      const childPrefixSegments = child.prefix.split(/(?=\/)/) // split on the slash separator but use a regex to lookahead and not actually match it, preserving it in the returned string segments
+      // split on the slash separator but use a regex to lookahead and not actually match it, preserving it in the returned string segments
+      const childPrefixSegments = child.prefix.split(pathRegExp)
       let cursor = flattened
       let parent
       for (const segment of childPrefixSegments) {
@@ -69,7 +228,6 @@ function flattenNode (flattened, node) {
           parent.children[segment] = cursor
         }
       }
-
       flattenNode(cursor, child)
     }
   }
@@ -95,4 +253,4 @@ function compressFlattenedNode (flattenedNode) {
   return flattenedNode
 }
 
-module.exports = { flattenNode, compressFlattenedNode, prettyPrintFlattenedNode }
+module.exports = { flattenNode, compressFlattenedNode, prettyPrintFlattenedNode, prettyPrintRoutesArray }
diff --git a/node.js b/node.js
index d8f9e48..adc5281 100644
--- a/node.js
+++ b/node.js
@@ -120,6 +120,7 @@ Node.prototype.split = function (length) {
       prefix: this.prefix.slice(length),
       children: this.children,
       kind: this.kind,
+      method: this.method,
       handlers: this.handlers.slice(0),
       regex: this.regex,
       constrainer: this.constrainer,


Test patch:
diff --git a/test/issue-182.test.js b/test/issue-182.test.js
new file mode 100644
index 0000000..869f9e7
--- /dev/null
+++ b/test/issue-182.test.js
@@ -0,0 +1,19 @@
+'use strict'
+
+const t = require('tap')
+const test = t.test
+const FindMyWay = require('..')
+
+test('Set method property when splitting node', t => {
+  t.plan(1)
+  const findMyWay = FindMyWay()
+
+  function handler (req, res, params) {
+    t.pass()
+  }
+
+  findMyWay.on('GET', '/health-a/health', handler)
+  findMyWay.on('GET', '/health-b/health', handler)
+
+  t.notMatch(findMyWay.prettyPrint(), /undefined/)
+})
diff --git a/test/pretty-print.test.js b/test/pretty-print.test.js
index 186ea74..08c0476 100644
--- a/test/pretty-print.test.js
+++ b/test/pretty-print.test.js
@@ -122,12 +122,119 @@ test('pretty print - constrained parametric routes', t => {
   const tree = findMyWay.prettyPrint()
 
   const expected = `└── /test (GET)
-    /test (GET {"host":"auth.fastify.io"})
+    /test (GET) {"host":"auth.fastify.io"}
     └── /:hello (GET)
-        :hello (GET {"version":"1.1.2"})
-        :hello (GET {"version":"2.0.0"})
+        /:hello (GET) {"version":"1.1.2"}
+        /:hello (GET) {"version":"2.0.0"}
 `
 
   t.is(typeof tree, 'string')
   t.equal(tree, expected)
 })
+
+test('pretty print - multiple parameters are drawn appropriately', t => {
+  t.plan(2)
+
+  const findMyWay = FindMyWay()
+  findMyWay.on('GET', '/test', () => {})
+  // routes with a nested parameter (i.e. no handler for the /:param) were breaking the display
+  findMyWay.on('GET', '/test/:hello/there/:ladies', () => {})
+  findMyWay.on('GET', '/test/:hello/there/:ladies/and/:gents', () => {})
+  findMyWay.on('GET', '/test/are/:you/:ready/to/:rock', () => {})
+
+  const tree = findMyWay.prettyPrint()
+
+  const expected = `└── /test (GET)
+    └── /
+        ├── :hello/there/:ladies (GET)
+        │   └── /and/:gents (GET)
+        └── are/:you/:ready/to/:rock (GET)
+`
+
+  t.is(typeof tree, 'string')
+  t.equal(tree, expected)
+})
+
+test('pretty print commonPrefix - use routes array to draw flattened routes', t => {
+  t.plan(4)
+
+  const findMyWay = FindMyWay()
+
+  findMyWay.on('GET', '/test', () => {})
+  findMyWay.on('GET', '/test/hello', () => {})
+  findMyWay.on('GET', '/testing', () => {})
+  findMyWay.on('GET', '/testing/:param', () => {})
+  findMyWay.on('PUT', '/update', () => {})
+
+  const radixTree = findMyWay.prettyPrint({ commonPrefix: true })
+  const arrayTree = findMyWay.prettyPrint({ commonPrefix: false })
+
+  const radixExpected = `└── /
+    ├── test (GET)
+    │   ├── /hello (GET)
+    │   └── ing (GET)
+    │       └── /:param (GET)
+    └── update (PUT)
+`
+  const arrayExpected = `└── / (-)
+    ├── test (GET)
+    │   └── /hello (GET)
+    ├── testing (GET)
+    │   └── /:param (GET)
+    └── update (PUT)
+`
+
+  t.is(typeof radixTree, 'string')
+  t.is(typeof arrayTree, 'string')
+
+  t.equal(radixTree, radixExpected)
+  t.equal(arrayTree, arrayExpected)
+})
+
+test('pretty print commonPrefix - handle wildcard root', t => {
+  t.plan(2)
+
+  const findMyWay = FindMyWay()
+
+  findMyWay.on('OPTIONS', '*', () => {})
+  findMyWay.on('GET', '/test/hello', () => {})
+  findMyWay.on('GET', '/testing', () => {})
+  findMyWay.on('GET', '/testing/:param', () => {})
+  findMyWay.on('PUT', '/update', () => {})
+
+  const arrayTree = findMyWay.prettyPrint({ commonPrefix: false })
+  const arrayExpected = `├── * (OPTIONS)
+└── / (-)
+    ├── test/hello (GET)
+    ├── testing (GET)
+    │   └── /:param (GET)
+    └── update (PUT)
+`
+
+  t.is(typeof arrayTree, 'string')
+  t.equal(arrayTree, arrayExpected)
+})
+
+test('pretty print commonPrefix - handle constrained routes', t => {
+  t.plan(2)
+
+  const findMyWay = FindMyWay()
+
+  findMyWay.on('GET', '/test', () => {})
+  findMyWay.on('GET', '/test', { constraints: { host: 'auth.fastify.io' } }, () => {})
+  findMyWay.on('GET', '/test/:hello', () => {})
+  findMyWay.on('PUT', '/test/:hello', () => {})
+  findMyWay.on('GET', '/test/:hello', { constraints: { version: '1.1.2' } }, () => {})
+  findMyWay.on('GET', '/test/:hello', { constraints: { version: '2.0.0' } }, () => {})
+
+  const arrayTree = findMyWay.prettyPrint({ commonPrefix: false })
+  const arrayExpected = `└── / (-)
+    └── test (GET)
+        test (GET) {"host":"auth.fastify.io"}
+        └── /:hello (GET, PUT)
+            /:hello (GET) {"version":"1.1.2"}
+            /:hello (GET) {"version":"2.0.0"}
+`
+  t.is(typeof arrayTree, 'string')
+  t.equal(arrayTree, arrayExpected)
+})