Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make feathers-hooks part of core and service events a hook #408

Closed
daffl opened this issue Sep 15, 2016 · 7 comments · Fixed by #697
Closed

Make feathers-hooks part of core and service events a hook #408

daffl opened this issue Sep 15, 2016 · 7 comments · Fixed by #697

Comments

@daffl
Copy link
Member

daffl commented Sep 15, 2016

The feathers-hooks plugin has become such an essential part of Feathers that it should probably become a core dependency. A good time to implement this is when removing callback support in both, hooks and services in Feathers v3.

This would also allow us to improve the service event dispatching by turning it into a hook (that always runs last) instead of a separate mixin (where the same functionality is currently provided by a different dependency Rubberduck).

Related issues:

@daffl daffl added this to the Buzzard milestone Sep 15, 2016
@daffl daffl changed the title Make feathers-hooks part of core and event dispatching a hook Make feathers-hooks part of core and service events a hook Sep 15, 2016
@beeplin
Copy link
Contributor

beeplin commented Oct 6, 2016

👍
Agree that the more quality hooks integrated into core, the better. Hooks are too vital to be scattered all around, and then we won't have to write or search for them by ourselves any more. :)

and also agree that server-event-emitting can be treated as a kind of hooks. Actually I found hooks and server-event-filters are quite similar: hooks get something done under some situations; filters get something(event broadcasting) not done under some situations.

Before the filters become hooks, I would like to suggest adding 'feather generate filter' to the cli tool, since I find myself always writing the same filter structuring code (just like hooks/index.js) over and over again.

@daffl
Copy link
Member Author

daffl commented Oct 7, 2016

Filters (with some sane defaults) are definitely going to be added in the next version of the CLI.

I don't think we'll add any built in hooks to core, instead it probably makes most sense to consolidate them all in https://github.com/feathersjs/feathers-hooks-common (unless they are specific to the plugin, e.g. for feathers-authentication).

@petermikitsh
Copy link
Contributor

This sounds like a fantastic idea, treating service events as hooks. I have a use case where I only want to send changed properties over the wire for patched service events. If the filter functions were hooks, it might make that easier to implement.

@subodhpareek18
Copy link

subodhpareek18 commented Feb 5, 2017

I have a use case where I only want to send changed properties over the wire for patched service events. If the filter functions were hooks, it might make that easier to implement.

@petermikitsh I have the exact use case. Were you able to figure out a solution for this with the current system?

@daffl
Copy link
Member Author

daffl commented Feb 5, 2017

You can already create a hook that does a diff with the previous data like this:

app.service('myservice').hooks({
  before: {
    patch(hook) {
      return hook.service.get(hook.id).then(previous => {
        hook.previousData = previous;

        return hook;
      });
    }
  },

  after: {
    patch(hook) {
      hook.result = diff(hook.result, hook.previousData);
    }
  }
});

@eddyystop
Copy link
Contributor

I've been happy with npm deep-diff for diff'ing nested objects.

@lock
Copy link

lock bot commented Feb 7, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants