sindresorhus / hook-std

Hook and modify stdout and stderr
MIT License
54 stars 12 forks source link

Promisify the API #7

Closed SamVerschueren closed 6 years ago

SamVerschueren commented 8 years ago

As discussed here https://github.com/sindresorhus/promise-time/pull/3#discussion_r71007913 it would be nice if the API returned a promise instead of working with callbacks.

jamestalmage commented 8 years ago

That really only works with the once method, right?

The default API needs to be callbacks because they get called multiple times.

sindresorhus commented 8 years ago

Ah right. Forgot about that case. I would personally usually only need either once or all, so I would focus on making that simple. Both of those could return a Promise. Not entirely sure about the API though.

Braindump:

hookStd({
    // defaults
    stream: process.stdout,
    first: false // like a `once` option
    silent: true
}).then();

// current functionality
hookStd.on('stdout', () => {});
hookStd.on('stderr', () => {});
hookStd.on('all', () => {}); // both stdout and stderr. ?
hookStd.once('stdout', () => {}); // only first

// Observables instead??
sindresorhus commented 7 years ago

New proposal:

We keep the existing functionality, but instead of returning an unhook function, we return a Promise with a .unhook() method. The Promise resolves when the unhook method is called. That way we can still have the flexibility of the callbacks, but also being able to await when the hooking is done. We should rename the callback argument to something like transform in the docs to reduce confusion.

const instance = hookStd(output => {
    instance.unhook();

    assert.strictEqual(output.trim(), 'unicorn');
});

await instance;

// `instance` is a promise with an extra property:
// `instance.unhook()` - to unhook
// `instance` resolves when the hook is unhooked

As a shorthand, we also return the unhook() method as a second argument:

await hookStd((output, unhook) => {
    unhook();

    assert.strictEqual(output.trim(), 'unicorn');
});

We can discuss support for Observables in a separate issue.

acostalima commented 7 years ago

@sindresorhus I've just submitted PR #13