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

Added support for multiple streams & optional streams per logger. Fixes #26 #27

Merged
merged 2 commits into from
May 30, 2018

Conversation

jordrake
Copy link
Contributor

@jordrake jordrake commented May 29, 2018

This would allow you to pass a stream as a property when configuring types to use that stream over the default one.

This does add a property to types that in its current state could not be represented by the config in package.json. If you feel this is necessary I can change it that type.stream is stream identifier and then have an api-only option of a map of identifier to implementation.

If you are happy with the current implementation I will crack on with updating the documentation.

Also, love the library in both functionality and ease of contribution!

EDIT: This now also fixes #26 by providing the ability to have an array of streams as either options.stream or type.stream

@klaudiosinani klaudiosinani self-requested a review May 29, 2018 23:06
@klaudiosinani klaudiosinani added the enhancement New feature or request label May 29, 2018
signale.js Outdated
_log(message) {
this._stream.write(message + '\n');
_log(message, stream = this._stream) {
stream.write(message + '\n');
Copy link
Owner

@klaudiosinani klaudiosinani May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea! I can see how this might be useful, but we can apply it in a way that supports multiple writable streams, in order to also close #26. The user will be able to define an array of valid writable streams under the stream logger attribute, which will override, for only that logger, both the default process.stdout as well as any other user-defined stream used when initializing a new Signale instance e.g.

const options = {
  stream: [stream_1, stream_2], // Overrides the default `process.stdout` with `stream_1` & `stream_2` for all loggers
  types: {
    error: {
     stream: [stream_3, stream_4] // Overrides both the default `process.stdout` and `stream_1` & `stream_2` with `stream_3` & `stream_4` only for the .error() logger
    }
  }
}
const signale = new Signale(options);
signale.error('Foo'); // 'Foo' will appear only on `stream_3` & `stream_4`
signale.success('Bar'); // 'Bar' will appear only on `stream_1` & `stream_2`

A new _formatStream(stream) method can be created on line 87, just above _formatDate(), that will make sure that the user-defined streams are packed into an array:

_formatStream(stream) {
  return Array.isArray(stream) ? stream : [stream];
}

And then we can write on all streams like so:

_log(message, streams = this._stream) {
  this._formatStream(streams).forEach(stream => {
    stream.write(message + '\n');
  });
}

Copy link
Contributor Author

@jordrake jordrake May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I agree it would be an easy win to fix #26 too!

I've done the work requested however I didn't extract the array type consolidation to a function as it's a single line only used in one place and it didn't feel like it fit in the same vein as the other format* functions which were string formatting. Happy to change this if you want though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A separate format function would be useful if access to writable streams was required on more than one different parts of the code and also for keeping things as modular as possible, but it is a change easily applied if needed in the future. Also, it would be better if the readme update could go to a separate pull request on its own, so it can be easily merged when ready to release the new version. Everything else looks awesome! Thank you a lot for taking the time to contribute : )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I've introduced both the format function and raised the README pr here: #29

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small thing I thought of was errors when trying to write to a stream, for multiple streams do you want to fail fast on the first or try to write to them all and failed at the end, e.g.:

_log(message, streams = this._stream) {
  let firstError;
  this._formatStream(streams).forEach(stream => {
    try {
      stream.write(message + '\n');
    } catch(e) {
      firstError = firstError || e;
    }
  });

  if(firstError) {
    throw firstError;
  }
}

Copy link
Owner

@klaudiosinani klaudiosinani May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I believe fast failure is better since it is up to the user to provide the valid writable streams and resolve any possible issues at first sight. Just a final tweak here and it is good to be merged. Also, thank you a lot for your readme PR!

@@ -78,11 +78,11 @@ class Signale {
}

_logger(type, ...messageObj) {
this._log(this._buildSignale(this._types[type], ...messageObj));
this._log(this._buildSignale(this._types[type], ...messageObj), this._types[type].stream);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice!

types.js Outdated
@@ -5,7 +5,8 @@ module.exports = {
error: {
badge: figures.cross,
color: 'red',
label: 'error'
label: 'error',
stream: process.stderr
Copy link
Owner

@klaudiosinani klaudiosinani May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be omitted, in order to prevent confusion, since default loggers are all expected to write to the default process.stdout stream

Copy link
Owner

@klaudiosinani klaudiosinani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few tweaks and it is good to go : )

@klaudiosinani klaudiosinani changed the title feat: optional stream per type Added support for multiple streams & optional streams per logger. Fixes #26 May 29, 2018
signale.js Outdated
});
}

_formatStream() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stream is undefined here, should be: _formatStream(stream)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good 👀

@jordrake jordrake force-pushed the stream-per-type branch 2 times, most recently from db06a9d to 8fa07a0 Compare May 30, 2018 15:41
@klaudiosinani klaudiosinani merged commit 608f882 into klaudiosinani:master May 30, 2018
@klaudiosinani
Copy link
Owner

Awesome! Thank you a lot! : )

@brettimus
Copy link
Contributor

Does this still need a blurb in the readme? Or do you prefer to update readme with an official release?

@klaudiosinani
Copy link
Owner

Yep, PR #29 will be merged just before the release of 1.2.0 : )

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

Successfully merging this pull request may close these issues.

Multiple write streams?
3 participants