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

Return listener-remover instead of response. #16

Open
nickclaw opened this issue May 20, 2016 · 1 comment
Open

Return listener-remover instead of response. #16

nickclaw opened this issue May 20, 2016 · 1 comment
Assignees

Comments

@nickclaw
Copy link

Currently onFinished behaves like this:

const onFinished = require('on-finished');
const app = require('express')();

app.use((req, res, next) => {
    const result = onFinished(res, () => /* code */);
    result === res; // true - just not very useful
    next();
});

What could be more useful is:

const onFinished = require('on-finished');
const app = require('express')();

app.use((req, res, next) => {
    const stopListening = onFinished(res, () => /* code */);

    doSomeOperation(err => {
        if (err) {
            stopListening();
            res.sendStatus(500); // onFinished listener is not called
        } else {
            next();
        }
    });
});

My actual use case is a little more complicated, (e.g. I can't just start listening if there isn't an error) and I know you could do this with a flag, but I'd argue in the case where you might have lots of onFinished listeners that need to stop listening, it's cleaner to give them a way to clean themselves up instead of just sitting in memory.

@dougwilson dougwilson self-assigned this Oct 28, 2016
@dougwilson
Copy link
Contributor

Hi @nickclaw currently this module's goal is to act as similar to an event emitter interface as possible (which is why we picked the module to have the name on-*, to look like .on( in event emitters. When you attach a listener, it returns the this, which is why we did that here. That at least explains the explanation as to why we are returning the "unless" value of the message back.

That said, the event emitters can remove listeners, but this does not let you--a feature gap I suppose. I would be open to if someone wanted to put together a PR to add this feature: add another export .remove or something similar and then it accepts msg and fn and will remove the fn listener from the given msg, just like .removeListener does on event emitters.

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

No branches or pull requests

2 participants