balena-io-modules / resin-device-logs

Resin.io device logs utilities.
Apache License 2.0
3 stars 2 forks source link

Add support for Supervisor's new log format #13

Closed jviotti closed 8 years ago

jviotti commented 8 years ago

See: https://github.com/resin-io/resin-ui/pull/15/files#diff-5f20d8bc11cc7e133d09556029eba893R34 See: https://github.com/resin-io/resin-supervisor/pull/213 Signed-off-by: Juan Cruz Viotti jviottidc@gmail.com

pcarranzav commented 8 years ago

@jviotti you said here that this is a breaking change, but it looks like the public API is maintained? i.e. this still emits a 'line' event for every log line, doesn't it?

jviotti commented 8 years ago

@pcarranzav The previous implementation only considered string payloads, and thus was not concerned with isSystem and timestamp. The module still emits a line event, but it now passes an object, including a message, property, etc; instead of just a string.

Of course this is not strictly necessary. I could emit back only the .message property to keep the interface, but this looks like a good opportunity to actually make this change. What do you think?

Page- commented 8 years ago

Is might be good to keep the current version as a wrapper around the object format, so basically it would be

logs.subscribe = ->
    emitter = new EventEmitter();
    logs.subscribeObjs(arguments...).on 'line', (line) =>
        emitter.emit('line', line.message)

As for this being a good time to break the interface, I would say it's a bad time because this means that anyone using the current version will have to change their code to be able to access logs from newer supervisors, causing their code to require changes to be able to work - rather than being able to just npm update to be able to handle newer supervisors

jviotti commented 8 years ago

Fair enough. Let's do this: I'll update this PR so only the message property is emitted back from the line event, so the interface is preserved, and once this is released, I'll create a new PR introducing the breaking change. Thus, the users will get the new change without needing to manually upgrade.

emirotin commented 8 years ago

:+1:

On Sun, Jul 24, 2016 at 10:18 PM, Juan Cruz Viotti <notifications@github.com

wrote:

Fair enough. Let's do this: I'll update this PR so only the message property is emitted back from the line event, so the interface is preserved, and once this is released, I'll create a new PR introducing the breaking change. Thus, the users will get the new change without needing to manually upgrade.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/resin-io/resin-device-logs/pull/13#issuecomment-234796360, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgGCI5z9HBBQXG1MoxsBdCzoZLKuZOMks5qY7qigaJpZM4JSzsR .

Eugene Mirotin Senior Frontend Engineer site: Resin.io https://resin.io/, twitter: @resin_io https://twitter.com/resin_io

jviotti commented 8 years ago

I've just released resin-device-logs@2.1.0 to npm.