Upgrade Winston

[MatrixFrog]

Didn't really test this other than running npm run test; let me know if there's anything else I should try out.

[MatrixFrog]

I think this should be good to go, let me know if you have any other questions on it!

[kibertoad]

Thank you! Will release a new version today.

[2naive]

@kibertoad still waiting :)

[kibertoad]

ouch. will do it right now, thank you for the ping.

[kibertoad]

Released in 4.0.3

[2naive]

@kibertoad thanks,
but @MatrixFrog I cannot confirm it fixes #1077

NODE_OPTIONS='--trace-warnings' forever list
info:    No forever processes running
(node:1836) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
    at emitCircularRequireWarning (node:internal/modules/cjs/loader:707:11)
    at Object.get (node:internal/modules/cjs/loader:721:5)
    at Object.exports.setLevels (/usr/local/lib/node_modules/forever/node_modules/cliff/node_modules/winston/lib/winston/common.js:32:14)
    at Object.<anonymous> (/usr/local/lib/node_modules/forever/node_modules/cliff/node_modules/winston/lib/winston.js:83:8)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
(node:1836) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
    at emitCircularRequireWarning (node:internal/modules/cjs/loader:707:11)
    at Object.get (node:internal/modules/cjs/loader:721:5)
    at Object.exports.setLevels (/usr/local/lib/node_modules/forever/node_modules/broadway/node_modules/winston/lib/winston/common.js:32:14)
    at Object.<anonymous> (/usr/local/lib/node_modules/forever/node_modules/broadway/node_modules/winston/lib/winston.js:82:8)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)

node -v
v16.13.2
npm view forever
forever@4.0.3

[kibertoad]

/usr/local/lib/node_modules/forever/node_modules/cliff/node_modules/winston/lib/winston.js

Looks like it is coming as a transitive dependency from cliff, so we'd need to get rid of that too.

[2naive]

@kibertoad and broadway

[kibertoad]

pretty sure we removed broadway already, is it coming as transitive from somewhere too?

[2naive]

@kibertoad
/usr/local/lib/node_modules/forever/node_modules/broadway/node_modules/winston/lib/winston.js

as you may see in second warning above

[kibertoad]

yeah, looks like flatiron is bringing it

[2naive]

A bit deeper: #1077 (comment)

@MatrixFrog maybe you could solve this

[MatrixFrog]

Well I can certainly try. The hardest part might be getting package owners to respond and merge stuff. Or maybe it's easier to just fork things as needed.

[2naive]

I suppose if @kibertoad will change dependency -> would be better just to fork:
Broadway won't fallback to 0.x and Flatiron seems too dead.

[kibertoad]

yeah, I'm happy to change to forks

[mmrwoods]

@MatrixFrog This seems to change in the output from forever --plain list, in a way that breaks forever-service on Amazon Linux 2 (and presumably other RHEL derivatives).

With forever v4.0.2, I get output like this:

info:    Forever processes running
data:        uid        command       script  forever pid  id logfile                 uptime
data:    [0] contentapi /usr/bin/node bin/www 7873    7927    /var/log/contentapi.log 75:20:4:18.05599999986589

With forever v4.0.3, I get output like this:

info:    Forever processes running
data:undefined    uid        command       script  forever pid  id logfile                 uptime
data:undefined[0] contentapi /usr/bin/node bin/www 8044    8113    /var/log/contentapi.log 0:0:16:20.09299999999996

That undefined after data: breaks the init script created by forever-service as it relies on a regex matching whitespace after data: to identify running services.

[MatrixFrog]

I see that too on my machine (Ubuntu 21.10). Hopefully I'll have some time to look into it this week.

[MatrixFrog]

It looks like the undefined is getting tacked on here https://github.com/winstonjs/logform/blob/master/pad-levels.js#L64

Quick fix might be to switch 'data' to 'info' at: https://github.com/foreversd/forever/blob/master/lib/forever/cli.js#L433 though of course you still might have to update your regex :(

[mmrwoods]

Thanks @MatrixFrog, I'm just a user of forever and forever-service, and for now I have pinned forever to 4.0.2, but I'll open an issue on forever-service and reference these comments.

[MatrixFrog]

filed winstonjs/winston#2053 -- I imagine the fix would be pretty easy if I was a little more familiar with the internals of Winston and its related packages.

[MatrixFrog]

Proposed fix is out: winstonjs/logform#135
I guess you can 👍 it and maybe that will help get it merged

[mmrwoods]

Thanks for submitting the Winston PR, would be good to see the issue fixed upstream. Just wondering though... is it not the responsibility of forever to specify the log levels it wants to be available before trying to log with those levels? Would a quick update to the forever.out logger to specify the log levels as winston.config.cli.levels (as per forever.log) fix this issue?

[mmrwoods]

btw, I have added a +1 to the PR, though I did also wonder about the fix... that code has seemingly been in use for 4 years and nobody else has raised this as an issue, and the winston readme also states that the default value for the levels param when calling createLogger() is winston.config.npm.levels.

I should point out that I am really not a javascript developer, so everything I say or suggest here should be taken with a healthy pinch of salt.

[MatrixFrog]

good point, i guess let's see what they say, and in the meantime yes it probably makes sense to pass the cli.levels in forever

[MatrixFrog]

Proposed fix on the forever side, here #1130