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

Add option forceConsole #2276

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Add option forceConsole #2276

merged 1 commit into from
Aug 6, 2024

Conversation

szolnokit
Copy link
Contributor

Add new option to the Console transport:
forceConsole (Boolean): when true, force transport to use console log/warn/error instead of write. With this option, VSCode terminal will work without setting the "outputCapture" to "std".

Add new option to the Console transport:
forceConsole (Boolean): when true, force transport to use console log/warn/error instead of write. With this option, VSCode terminal will work without setting the "outputCapture" to "std".
@wbt
Copy link
Contributor

wbt commented Feb 6, 2023

Hmm, this seems to change default behavior, which might require a major version bump. VSCode might be a better place for this to be fixed, especially if the impact is intended only for that one environment.

@coreyjpieper
Copy link

I believe this would resolve my feature request #2175 and others like it. I'm not sure how it would change the default behavior since the default value is false, so the statement:

if (console._stderr && !this.forceConsole)

would evaluate to the same as before if this prop wasn't set.

@neilenns
Copy link
Contributor

neilenns commented Jul 31, 2024

Found this open PR after spending way too long trying to find out why I wasn't getting Winston console logs in VSCode. Turns out it's because I'm attaching to the process (required in my case), and stderr/stdout can't be captured in that config. Setting "outputCapture": "std", as suggested in other linked issues is not a solution as that only works when VSCode is launching the node process, not when attaching to a running node process.

Would love to have this option available as it would enable VSCode to actually see the logs I want to write with Winston.

@DABH
Copy link
Contributor

DABH commented Aug 6, 2024

I agree that it doesn't look like this is introducing a breaking change. I'm okay with merging this but it would be good to have some documentation (even just a comment) somewhere explaining that you may need to set this option with VSCode. Enough people are using VSCode that maybe that's worth a line in the README? @neilenns , would you feel up to the task of adding some brief documentation somewhere for this?

(can be a separate PR - also anyone else reading this is more than welcome to do this 🙏)

@DABH DABH merged commit b2098fd into winstonjs:master Aug 6, 2024
@neilenns
Copy link
Contributor

neilenns commented Aug 6, 2024

Yep, I'd be happy to and will later today!

@DABH
Copy link
Contributor

DABH commented Aug 6, 2024

Cool, you rock!! 🎸

@Jimbly
Copy link

Jimbly commented Aug 7, 2024

I, like presumably many others, do console.log = logger.log.bind(logger) (etc), to capture all console.log calls from all modules into my logging stream (and previously use something like the SimpleConsoleTransport in #1544 to get the logs showing up in Node Inspector / VSCode / etc). Unfortunately this fix doesn't actually help (causes an infinite recursion / call stack exceeded crash). Probably just need to save a reference to the global console at the time of logger instantiation and it'd be good though?

@DABH
Copy link
Contributor

DABH commented Aug 8, 2024

@Jimbly @neilenns If you think saving a copy of the global console or some other mod to this PR would be an improvement, please open another quick PR and let's get those fixes/improvements in there. With a test case would be ideal, but let's just do what we can to address this properly once and for all, I know this and related issues like #1544 have been open for a while. Appreciate your help in figuring out all these corner cases and details.

@neilenns
Copy link
Contributor

neilenns commented Aug 8, 2024

I'm not an expert at all in this unfortunately, just someone who got tripped up with the whole "can't capture in vscode when attaching to node.js" mess.

@neilenns
Copy link
Contributor

neilenns commented Aug 8, 2024

If you think saving a copy of the global console or some other mod to this PR would be an improvement, please open another quick PR and let's get those fixes/improvements in there.

Looking at this a bit more, the changed proposed in #2474 might fix the infinite recursion issue @Jimbly hit? Since it doesn't call console.log() directly and instead uses inspector.console to log?

@neilenns
Copy link
Contributor

neilenns commented Aug 8, 2024

I manually applied a variation of the change in #2474 and confirmed it does not have any issue with infinite recursion. In my main .ts file I added this:

import mainLogger from "@utils/logger";
console.log = mainLogger.log.bind(mainLogger);

And everything logs correctly to the console.

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

Successfully merging this pull request may close these issues.

6 participants