Repository: delvedor/find-my-way
Pull request #43 (2017-11-17 11:30:33)

feat(off): add off feature
Add `.off(method, path)` method.

## Related issues

- https://github.com/delvedor/find-my-way/issues/40

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14251669/badge)](https://coveralls.io/builds/14251669)

Coverage decreased (-3.06%) to 95.426% when pulling **a61d137b7b0194d0ee0556d5015b39594e12a834 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14251683/badge)](https://coveralls.io/builds/14251683)

Coverage decreased (-2.6%) to 95.85% when pulling **07f539fd446ad4ca6c7bfa3a58f35137b8e34ce1 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14251683/badge)](https://coveralls.io/builds/14251683)

Coverage decreased (-2.6%) to 95.85% when pulling **07f539fd446ad4ca6c7bfa3a58f35137b8e34ce1 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14251707/badge)](https://coveralls.io/builds/14251707)

Coverage decreased (-2.6%) to 95.85% when pulling **07f539fd446ad4ca6c7bfa3a58f35137b8e34ce1 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

delvedor: Hi! Thank you for the proposal!

I think the best option would be regenerate the entire tree every time you call `.off`.
1. No need to change the internals which are highly optimized.
2. add the parent node to a Node means that you are creating a circular reference, in this way you can't print the tree (`JSON.stringify(router.tree, null, 2)`) which is very useful for debugging.

We can create an array where we save all the routes and if the user calls `.off` we remove the specific route from that array and then we regenerate the tree.
What do you think?
hekike: @delvedor thanks for the feedback. I'm okay with the solution what you proposed.
Do you want to work on it or should I go ahead?
delvedor: @hekike go ahead! :)
hekike: @delvedor done, how about now? ;)
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255047/badge)](https://coveralls.io/builds/14255047)

Coverage decreased (-0.1%) to 98.357% when pulling **26868bcdad4f6f4d7736dfa857f7782cf27f3298 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255213/badge)](https://coveralls.io/builds/14255213)

Coverage decreased (-0.1%) to 98.357% when pulling **2bff6caa486d43c6282c18498a07b53248a35814 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255356/badge)](https://coveralls.io/builds/14255356)

Coverage decreased (-0.1%) to 98.357% when pulling **e14bb2236af4d2429b0ae74d41fe17a547ca8f6d on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255356/badge)](https://coveralls.io/builds/14255356)

Coverage decreased (-0.1%) to 98.357% when pulling **e14bb2236af4d2429b0ae74d41fe17a547ca8f6d on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255356/badge)](https://coveralls.io/builds/14255356)

Coverage decreased (-0.1%) to 98.357% when pulling **e14bb2236af4d2429b0ae74d41fe17a547ca8f6d on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255356/badge)](https://coveralls.io/builds/14255356)

Coverage decreased (-0.1%) to 98.357% when pulling **e14bb2236af4d2429b0ae74d41fe17a547ca8f6d on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255539/badge)](https://coveralls.io/builds/14255539)

Coverage decreased (-0.1%) to 98.357% when pulling **61f87c4c94bd2bdb6bb2898a2e4b98e30858dc3a on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

delvedor: Try add this test:
```js
test('off with nested wildcards with parametric and static', t => {
  t.plan(2)
  const findMyWay = FindMyWay({
    defaultRoute: (req, res) => {
      t.fail('we should not be here, the url is: ' + req.url)
    }
  })

  findMyWay.on('GET', '*', (req, res, params) => {
    t.is(params['*'], '/foo2/first/second')
  })
  findMyWay.on('GET', '/foo1/*', () => {})
  findMyWay.on('GET', '/foo2/*', () => {})
  findMyWay.on('GET', '/foo3/:param', () => {})
  findMyWay.on('GET', '/foo3/*', () => {})
  findMyWay.on('GET', '/foo4/param/hello/test/long/route', () => {})

  findMyWay.off('GET', '/foo3/*')
  t.notOk(findMyWay.find('GET', '/foo3/first/second'))

  findMyWay.off('GET', '/foo2/*')
  findMyWay.lookup(
    { method: 'GET', url: '/foo2/first/second' },
    null
  )
})
```
For some reason is not passing on my machine.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14255648/badge)](https://coveralls.io/builds/14255648)

Coverage increased (+0.08%) to 98.563% when pulling **d5882f4345a39f597a4391b3563a7bbda35befe2 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

hekike: @delvedor are you sure it's related to the `.off`, for me, it's failing any `off` call.

```js
test('off with nested wildcards with parametric and static', t => {
  t.plan(1)
  const findMyWay = FindMyWay({
    defaultRoute: (req, res) => {
      t.fail('we should not be here, the url is: ' + req.url)
    }
  })

  findMyWay.on('GET', '*', (req, res, params) => {
    t.is(params['*'], '/foo2/first/second')
  })
  findMyWay.on('GET', '/foo1/*', () => {})
  findMyWay.on('GET', '/foo2/*', () => {})
  findMyWay.on('GET', '/foo3/:param', () => {})
  findMyWay.on('GET', '/foo3/*', () => {})
  findMyWay.on('GET', '/foo4/param/hello/test/long/route', () => {})

  // findMyWay.off('GET', '/foo3/*')
  // t.notOk(findMyWay.find('GET', '/foo3/first/second'))

  // findMyWay.off('GET', '/foo2/*')
  findMyWay.lookup(
    { method: 'GET', url: '/foo2/first/second' },
    null
  )
})
```

delvedor: @hekike this is the standard situation:
```js
test('check 1', t => {
  t.plan(1)
  const findMyWay = FindMyWay({
    defaultRoute: (req, res) => {
      t.fail('we should not be here, the url is: ' + req.url)
    }
  })

  findMyWay.on('GET', '*', () => {})
  findMyWay.on('GET', '/foo1/*', () => {})
  findMyWay.on('GET', '/foo2/*', (req, res, params) => {
    t.is(params['*'], 'first/second')
  })
  findMyWay.on('GET', '/foo3/:param', () => {})
  findMyWay.on('GET', '/foo3/*', () => {})
  findMyWay.on('GET', '/foo4/param/hello/test/long/route', () => {})

  findMyWay.lookup(
    { method: 'GET', url: '/foo2/first/second' },
    null
  )
})
```

And this is the one without `/foo2/*`:
```js
test('check 2', t => {
  t.plan(1)
  const findMyWay = FindMyWay({
    defaultRoute: (req, res) => {
      t.fail('we should not be here, the url is: ' + req.url)
    }
  })

  findMyWay.on('GET', '*', (req, res, params) => {
    t.is(params['*'], '/foo2/first/second')
  })
  findMyWay.on('GET', '/foo1/*', () => {})
  findMyWay.on('GET', '/foo3/:param', () => {})
  findMyWay.on('GET', '/foo3/*', () => {})
  findMyWay.on('GET', '/foo4/param/hello/test/long/route', () => {})

  findMyWay.lookup(
    { method: 'GET', url: '/foo2/first/second' },
    null
  )
})
```
Both tests are passing on my machine, this means that `off` is messing up something :S
hekike: @delvedor I found the issue and fixed it. Thanks for the help!
hekike: @delvedor I also added your wildcard test case to the tests.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14256284/badge)](https://coveralls.io/builds/14256284)

Coverage increased (+0.08%) to 98.563% when pulling **15d7daa97165ef5d084d840bfb44563eaa8fa436 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14256284/badge)](https://coveralls.io/builds/14256284)

Coverage increased (+0.08%) to 98.563% when pulling **15d7daa97165ef5d084d840bfb44563eaa8fa436 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14256284/badge)](https://coveralls.io/builds/14256284)

Coverage increased (+0.08%) to 98.563% when pulling **15d7daa97165ef5d084d840bfb44563eaa8fa436 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14256284/badge)](https://coveralls.io/builds/14256284)

Coverage increased (+0.08%) to 98.563% when pulling **15d7daa97165ef5d084d840bfb44563eaa8fa436 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14256319/badge)](https://coveralls.io/builds/14256319)

Coverage increased (+0.08%) to 98.563% when pulling **73d94d79522058757fd97c55cd1f3a95234a0942 on hekike:feat/off** into **52c2eaab49097940aaef55b2333d14f167fa325b on delvedor:master**.

delvedor: @hekike you are welcome!
For the sake of curiosity, what did you changed?

Patch:
diff --git a/README.md b/README.md
index 0b512ef..669157e 100644
--- a/README.md
+++ b/README.md
@@ -117,6 +117,31 @@ parametric(regex)
 multi parametric(regex)
 ```
 
+<a name="off"></a>
+#### off(method, path)
+Deregister a route.
+```js
+router.off('GET', '/example')
+// => { handler: Function, params: Object, store: Object}
+// => null
+```
+
+##### off(methods[], path, handler, [store])
+Deregister a route for each method specified in the `methods` array.
+It comes handy when you need to deregister multiple routes with the same path but different methods.
+```js
+router.off(['GET', 'POST'], '/example')
+// => [{ handler: Function, params: Object, store: Object}]
+// => null
+```
+
+<a name="reset"></a>
+#### reset()
+Empty router.
+```js
+router.reset()
+```
+
 ##### Caveats
 * Since *static* routes have greater priority than *parametric* routes, when you register a static route and a dynamic route, which have part of their path equal, the static route shadows the parametric route, that becomes not accessible. For example:
 ```js
diff --git a/index.js b/index.js
index 90bbbe1..3228f5f 100644
--- a/index.js
+++ b/index.js
@@ -35,6 +35,7 @@ function Router (opts) {
   }
 
   this.tree = new Node()
+  this.routes = []
 }
 
 Router.prototype.on = function on (method, path, handler, store) {
@@ -58,6 +59,13 @@ Router.prototype.on = function on (method, path, handler, store) {
   const params = []
   var j = 0
 
+  this.routes.push({
+    method: method,
+    path: path,
+    handler: handler,
+    store: store
+  })
+
   for (var i = 0, len = path.length; i < len; i++) {
     // search for parametric or wildcard routes
     // parametric route
@@ -183,6 +191,38 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler
   }
 }
 
+Router.prototype.reset = function reset () {
+  this.tree = new Node()
+  this.routes = []
+}
+
+Router.prototype.off = function off (method, path) {
+  var self = this
+
+  if (Array.isArray(method)) {
+    return method.map(function (method) {
+      return self.off(method, path)
+    })
+  }
+
+  // method validation
+  assert(typeof method === 'string', 'Method should be a string')
+  assert(httpMethods.indexOf(method) !== -1, `Method '${method}' is not an http method.`)
+  // path validation
+  assert(typeof path === 'string', 'Path should be a string')
+  assert(path.length > 0, 'The path could not be empty')
+  assert(path[0] === '/' || path[0] === '*', 'The first character of a path should be `/` or `*`')
+
+  // Rebuild tree without the specific route
+  var newRoutes = self.routes.filter(function (route) {
+    return !(method === route.method && path === route.path)
+  })
+  self.reset()
+  newRoutes.forEach(function (route) {
+    self.on(route.method, route.path, route.handler, route.store)
+  })
+}
+
 Router.prototype.lookup = function lookup (req, res) {
   var handle = this.find(req.method, sanitizeUrl(req.url))
   if (!handle) return this._defaultRoute(req, res)


Test patch:
diff --git a/test/methods.test.js b/test/methods.test.js
index 53d919d..5b045a0 100644
--- a/test/methods.test.js
+++ b/test/methods.test.js
@@ -5,15 +5,45 @@ const test = t.test
 const FindMyWay = require('../')
 
 test('the router is an object with methods', t => {
-  t.plan(3)
+  t.plan(4)
 
   const findMyWay = FindMyWay()
 
   t.is(typeof findMyWay.on, 'function')
+  t.is(typeof findMyWay.off, 'function')
   t.is(typeof findMyWay.lookup, 'function')
   t.is(typeof findMyWay.find, 'function')
 })
 
+test('on throws for invalid method', t => {
+  t.plan(1)
+  const findMyWay = FindMyWay()
+
+  t.throws(() => {
+    findMyWay.on('INVALID', '/a/b')
+  })
+})
+
+test('on throws for invalid path', t => {
+  t.plan(3)
+  const findMyWay = FindMyWay()
+
+  // Non string
+  t.throws(() => {
+    findMyWay.on('GET', 1)
+  })
+
+  // Empty
+  t.throws(() => {
+    findMyWay.on('GET', '')
+  })
+
+  // Doesn't start with / or *
+  t.throws(() => {
+    findMyWay.on('GET', 'invalid')
+  })
+})
+
 test('register a route', t => {
   t.plan(1)
   const findMyWay = FindMyWay()
@@ -37,6 +67,124 @@ test('register a route with multiple methods', t => {
   findMyWay.lookup({ method: 'POST', url: '/test' }, null)
 })
 
+test('off throws for invalid method', t => {
+  t.plan(1)
+  const findMyWay = FindMyWay()
+
+  t.throws(() => {
+    findMyWay.off('INVALID', '/a/b')
+  })
+})
+
+test('off throws for invalid path', t => {
+  t.plan(3)
+  const findMyWay = FindMyWay()
+
+  // Non string
+  t.throws(() => {
+    findMyWay.off('GET', 1)
+  })
+
+  // Empty
+  t.throws(() => {
+    findMyWay.off('GET', '')
+  })
+
+  // Doesn't start with / or *
+  t.throws(() => {
+    findMyWay.off('GET', 'invalid')
+  })
+})
+
+test('off with nested wildcards with parametric and static', t => {
+  t.plan(3)
+  const findMyWay = FindMyWay({
+    defaultRoute: (req, res) => {
+      t.fail('we should not be here, the url is: ' + req.url)
+    }
+  })
+
+  findMyWay.on('GET', '*', (req, res, params) => {
+    t.is(params['*'], '/foo2/first/second')
+  })
+  findMyWay.on('GET', '/foo1/*', () => {})
+  findMyWay.on('GET', '/foo2/*', () => {})
+  findMyWay.on('GET', '/foo3/:param', () => {})
+  findMyWay.on('GET', '/foo3/*', () => {})
+  findMyWay.on('GET', '/foo4/param/hello/test/long/route', () => {})
+
+  var route1 = findMyWay.find('GET', '/foo3/first/second')
+  t.is(route1.params['*'], 'first/second')
+
+  findMyWay.off('GET', '/foo3/*')
+
+  var route2 = findMyWay.find('GET', '/foo3/first/second')
+  t.is(route2.params['*'], '/foo3/first/second')
+
+  findMyWay.off('GET', '/foo2/*')
+  findMyWay.lookup(
+    { method: 'GET', url: '/foo2/first/second' },
+    null
+  )
+})
+
+test('deregister a route without children', t => {
+  t.plan(2)
+  const findMyWay = FindMyWay()
+
+  findMyWay.on('GET', '/a', () => {})
+  findMyWay.on('GET', '/a/b', () => {})
+  findMyWay.off('GET', '/a/b')
+
+  t.ok(findMyWay.find('GET', '/a'))
+  t.notOk(findMyWay.find('GET', '/a/b'))
+})
+
+test('deregister a route with children', t => {
+  t.plan(2)
+  const findMyWay = FindMyWay()
+
+  findMyWay.on('GET', '/a', () => {})
+  findMyWay.on('GET', '/a/b', () => {})
+  findMyWay.off('GET', '/a')
+
+  t.notOk(findMyWay.find('GET', '/a'))
+  t.ok(findMyWay.find('GET', '/a/b'))
+})
+
+test('deregister a route by method', t => {
+  t.plan(2)
+  const findMyWay = FindMyWay()
+
+  findMyWay.on(['GET', 'POST'], '/a', () => {})
+  findMyWay.off('GET', '/a')
+
+  t.notOk(findMyWay.find('GET', '/a'))
+  t.ok(findMyWay.find('POST', '/a'))
+})
+
+test('deregister a route with multiple methods', t => {
+  t.plan(2)
+  const findMyWay = FindMyWay()
+
+  findMyWay.on(['GET', 'POST'], '/a', () => {})
+  findMyWay.off(['GET', 'POST'], '/a')
+
+  t.notOk(findMyWay.find('GET', '/a'))
+  t.notOk(findMyWay.find('POST', '/a'))
+})
+
+test('reset a router', t => {
+  t.plan(2)
+  const findMyWay = FindMyWay()
+
+  findMyWay.on(['GET', 'POST'], '/a', () => {})
+  findMyWay.reset()
+
+  t.notOk(findMyWay.find('GET', '/a'))
+  t.notOk(findMyWay.find('POST', '/a'))
+})
+
 test('default route', t => {
   t.plan(1)