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

Event emit skipping #862

Closed
wants to merge 1 commit into from
Closed

Event emit skipping #862

wants to merge 1 commit into from

Conversation

TimNZ
Copy link
Contributor

@TimNZ TimNZ commented Apr 28, 2018

Refer to discussion here:
feathersjs-ecosystem/feathers-sequelize#188

Want granular control over event emitting after service methods.

You'll likely want to tidy up my test which is hacky.
I'm not knowledgeable on how you test for something NOT happening.

@daffl
Copy link
Member

daffl commented Apr 28, 2018

Thank you for the PR @TimNZ. Skipping even emitting should however already be possible by returning feathers.SKIP in the last hook you want to run.

@TimNZ
Copy link
Contributor Author

TimNZ commented Apr 28, 2018

I knew about SKIP but didn't test well enough if it could achieve what I wanted.

Have tested nested service calls and a simple hook and modifying context/params achieves desired flow.

Though if any other 'finally' hooks ever get added in the future this will break them.
That's why I prefer the explicit event skipping, doesn't depend on any particular hook ordering

@TimNZ
Copy link
Contributor Author

TimNZ commented Apr 28, 2018

If you decide not to have explicit event skipping support,
perhaps a note in the SKIP section and Events page docs that SKIP prevents event emitting as well.
https://docs.feathersjs.com/api/hooks.html#returning-feathersskip

My event logging module is separate from hooks handling, I like the modular micro approach you've taken and don't want to introduce dependencies.

@daffl
Copy link
Member

daffl commented May 23, 2018

Haven't forgotten about this. Just figuring out if and how this will fit into the next release.

@TimNZ
Copy link
Contributor Author

TimNZ commented May 25, 2018

@daffl Thanks.

No rush.

A context flag to skip events processing, decoupled from hook flow, still makes most sense to me.

@daffl
Copy link
Member

daffl commented Aug 13, 2018

Allright, so what I'd like to allow for this release would be adding a hook context.event which will contain the service event name that will be sent once the hook completes successfully. That way it would be possible to change the event that is emitted or prevent any events from being sent by setting context.event = null.

@TimNZ
Copy link
Contributor Author

TimNZ commented Aug 13, 2018

Does the job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants