feathers-hooks-common: disable hook seems to have wrong true/false logic

Code here means to disable when the result is false, not to disable when the result is true.

if (typeof realm === 'function') {
    return function (hook) {
      const result = realm(hook);
      const update = check => {
        if (!check) {
          throw new errors.MethodNotAllowed(`Calling '${hook.method}' not allowed. (disable)`);
        }
      };
  }

But in docs here it seems to be opposite logic:

  // Disable the remove hook if the user is not an admin
  remove: hooks.disable(function(hook) {
    return !hook.params.user.isAdmin
  })

Either the doc or the code is wrong. I guest it’s the code which is wrong.

About this issue

Original URL
State: closed
Created 8 years ago
Comments: 16 (9 by maintainers)

Commits related to this issue

Added disableMethod to deprecate disbale hook. - disable hook has a predicate & we are deprecating predicates. - the disable predicate is implemented in perhaps the opposite sense. - In https://githu... — committed to feathersjs-ecosystem/feathers-hooks-common by eddyystop 7 years ago

Most upvoted comments

https://github.com/feathersjs/feathers-hooks-common/pull/108

The disableMethod hook deprecates disable in feathers-hooks-common v3.

eddyystop on Jan 18, 2017

I suggest we create another hook that works like disable but with the correct sense. We can wean people away from disable by stressing the new hook in docs and maybe adding a deprecated message.

Any suggestions for the name of the replacement hook?

p.s. I’ve suggested something similar be done with the remove hook which is a no-op for calls made by the server. I’ve suggested delete as the name of that replacement hook.

eddyystop on Jan 17, 2017

Thanks for your example. I assume it should be if (check) { but let me ask what the ramifications might be regarding changing the code.

As an aside, a number of hooks, like disable, allow their last param to be a function. This function indicates if the hook should be run or not. That feature will be deprecated in all these hooks and removed in the future.

I recommend you use hooksCommon.iff(condition, hook) where condition may be sync, or async using a Promise. iff(condition, hook1, hook2, ...) will be supported in the future.

eddyystop on Oct 31, 2016