Created support for event filter and permission hooks. Reorganized service hooks because of this.

[eddyystop]

[Edited for simplification and clarity]

• Eric needs some of the service hooks to work for event filters, especially the conditionals.
• I felt the conditional hooks should work for permissions as well.
• Duck typing would not work as service and permission hook funcs both have a signature of (hook).
• Filter and permission hooks were therefore implemented using higher order constructs.
• The service hooks in the repo were re-grouped into files which made sense.

hooks = require('feathers-hooks-common')

• Runs same code as it has previously. Nothing new introduced.

filterHooks = require('feathers-hooks-common/filters')

• Provides filterHooks.iffElse(predicate, [trueFilters], [falseFilters]);
  as well as iff, iff.else, when, unless and ifNot
• Provides filterHooks.some(...trueFalse) and every.
• Provides filterHooks.remove(), .pluck(), traverse() and the new setFilteredAt()
• These hooks, the predicate and [trueFilters] have signature (data, connection, hook).
• The [trueFilters] are dispatched one after another using the same code as
  https://github.com/feathersjs/feathers-socket-commons/blob/master/src/events.js#L41-L70
• The hooks return 'false' or modified data.

permissionHooks = require('feathers-hooks-common/permissions')

• Provides permissionHooks.iffElse(predicate, [truePermissions], [falsePermissions]);
  as well as iff, iff.else, when, unless and ifNot
• Provides permissionHooks.some(...trueFalse) and every.
• These hooks, the predicate and [truePermissions] have signature (hook).
• some and every may be used as either the predicate or [truePermissions].
• The [truePermissions] are dispatched in parallel. The result is true if they all returned true,
  i.e. they work file every().
• The hooks return a boolean.

hooks = require('feathers-hooks-common/hooks')

• All the same hooks as feathers-hooks-common but using the same same common code as filter
  and permission hooks.
• Code for hooks and tests now sensibly organized.
• The reorganization was cut/paste. The 350+ tests are detailed. I feel comfortable with the result.

Outstanding

• Do we leave the filter and permission hooks here or bundle them from elsewhere?
• Export https://github.com/feathersjs/feathers-socket-commons/blob/master/src/events.js#L41-L70 so a copy of this code in src/filters/conditional.js can be replaced by an import. That'll keep filter dispatching DRY.

Which hooks to use?

• I suggest we change feathers-hooks-common/index.js to require feathers-hooks-common/hooks.
• The existing hooks are packaged the way they are for historical reasons. Not logically.
• The tests are a bag of files.
• The hooks were repackaged to support the filer and permission higher order functions.
  The packaging is sensible and follows the layout of the docs.
• The reganization was cut/paste. The 350+ tests are detailed. I feel comfortable with the reorganization.

[eddyystop]

Code Climate is complaining about 'similar' 3 line pieces of code. The code would be harder to read if I refactored. Alternatively there would be going on a dozen fingerprints to file.

I want to suppress similar code checking for this repo.

[eddyystop]

[Removed]

[ekryski]

Do we leave the filter and permission hooks here or bundle them from elsewhere?

I think they probably should be their own repo but this is a good stop for now. Nice work!

Nice work re-organizing the files. That was a good call and was something I was going to do when I had some time.

On visual inspection it looks all good. I won't be able to pull this down and run it until this evening but if it's all good I'll merge and release (if that's cool with you @eddyystop) and then try and get feathers-permissions released as well.

[eddyystop]

There are some current hooks whose last param is an optional predicate func. That option displays a deprecated message, and the option was not documented since before I joined up here.

Full disclosure: I removed those options in feathers-hooks-common/hooks. I can make changes to remain backward compatible if that's necessary,.

[daffl]

I feel like I'm missing something, what was the reason for adding 8500 lines of code for supporting filters? I see there is some reorganization but it looks like all the old things are still there. Also, would it make more sense to just have each hook in their own file and not have to worry about how to organize them (that's what I was going for eventually)?

[eddyystop]

The idea is to delete the old hook files and use the new, reorganized files.

I can break the new hook files into one per hook. There will be something like 80 of them.

[daffl]

I think it makes sense since the test files are already mostly individual per hook and it'll be easier for everybody to know where to find what. The only other thing I was going to suggest is to use dash-separated instead of camelCased filenames. It might not be a super common problem but I had some trouble with Git, camelCased filenames and case-insensitive vs case sensitive file systems (Windows vs. Unix) before.

[ekryski]

I'm down with having 1 hook per file and would prefer it but I don't think it should stop this from shipping. That's some thing that really is internal to the module and doesn't affect the public API. In fact, I'd actually love to have all hook functions exposed top level (even utils) so that we can just do:

import {checkContext} from feathers-hooks-common;

Again, not something that should happen in this PR.

[eddyystop]

I'd actually love to have all hook functions exposed top level (even utils) so that we can just do:

These changes do that.

[eddyystop]

There is a related issue I'd like to address.

• $client, waiting 16 days: add $client hook #65 despite multiple requests for review.
• validateSchema, waiting 15 days: Added validateSchema hook to validate JSON objects #68 despite multiple requests for review.
• this issue, waiting 6 days before daffl commented

I think this is too tardy even for an approaching holiday season.

• I'm starting to get (minor) merge conflicts with myself.
• I have code waiting which assumes un-reviewed code will land.
• I have to keep a list of what else to do once each PR lands.
• I'm losing the fun factor.

Can we make go, no go decisions on the above 3 PRs?

I'll then break the hooks down to individual files. I suggest they remain under the folders as in this PR rather than a bag of 80 files. These folders tie into the structure of the docs.

[daffl]

I just noticed this one because it was still open and had a huge number of additions.

In general, if it's a non-breaking new feature (#63) I don't think there is anything wrong with just pushing it out and iterating.

For #65 nobody has replied to my last comment.

[eddyystop]

So I can take that as a ?

[daffl]

It seems like a huge amount of additions to me which also makes it really hard to review but if it's what @ekryski needs then sure.

[ekryski]

I'm looking this afternoon for real. Will come back with comments. @daffl from what I can see the majority of the changes are just restructuring.

[daffl]

It looks like it's mostly additions and no deletions which what I was wondering about.

@eddyystop Should we move each hook into their own files? I think it'll be the best and most straightforward way of structuring it. I can make a PR for it.

[eddyystop]

Once this PR is approved my intent was, first, to remove the old files like new.js and common.js and have src/index.js inherit from src/hooks/index.js, thereby pointing to the new files like alter-data.js. These new files share code in src/common/ with the hooks for filters and permissions. I've already mentioned removing the old files above.

Second, to break the new files into files for individual hooks.

I've already started on this breaking out of the files but stopped part way through because this PR was going nowhere and has enough PRs added to it.

[ekryski]

@eddyystop !!!!! 😄

Sorry it's taken me so long to review this properly but it looks good. Lemme know when you have another PR up to remove the old files and we'll get that merged ASAP. I think having the old files around and mixing new functionality with a re-structuring is what made this a harder review. Maybe in the future we can try and break those up into separate PRs.

Really nice work!