#249 Add explicit info logger to avoid writing info logs to stderr

[ThYpHo0n]

This relates to #249 and should fix it BUT I didn't got the integration tests running on my mac so please review and test this before merge!

[johnnyreilly]

Well the CI tests pass which is a good sign given they run on Linux (Travis) and Windows (AppVeyor) @ThYpHo0n could you tell me what problems you experienced running the integration tests on your mac? Tests ought to work there as well. If the tests don't run on mac then I'd like to fix that as a matter of priority. Would you be able to open a separate issue for that if that is the case?

Onto the review:

As well as relating to #249 this relates to #95. I've read through #95 and it seems that the choice to use stderr (as presently) or stdout as ts-loader used to is somewhat arbitrary.

The advantage of pumping to stderr (as I understand it) is only that when running in silent you still see the log message at startup (eg ts-loader: Using typescript@2.0.3 and c:\source\project-name\tsconfig.json)

But given that this change was introduced to ts-loader purely to satisfy @alexdrel, and that more than one person (@ThYpHo0n and @SteveSandersonMS at least) find it problematic I'm inclined to take this PR and revert the behaviour.

I think everyone relevant to this has been included on this comment - please could you get in contact if you have any views on this.

cc @jbrantly

[ThYpHo0n]

@johnnyreilly thanks for your reply. I've created #314.

My cents to the review:
As far as I understood the code correctly you don't see anything if silent=true. It just doesn't log anything. I see the point of @alexdrel that (on the other way round) errors shouldn't be logged at stdout. That's why I've added a dedicated info logger to separate both concerns.

[johnnyreilly]

Great thanks - let's see if anyone has any alternative viewpoints. If no one responds then it looks reasonable to merge this.

[jbrantly]

The problem with this is that it would effectively break the scenario reported in #95. That is, running something like webpack --progress -j > m.json would break. That said, I also get that writing to stderr causes other issues, particularly in Windows environments. I'm not sure that there's a good one-size-fits-all solution here. Maybe the solution is a flag to toggle between stderr and stdout?

[johnnyreilly]

For anyone else wondering (I was) the -j flag is the shorthand for the json flag: https://github.com/webpack/docs/wiki/cli

Write JSON to stdout instead of a human readable format.

As to having a flag to toggle this; well that sounds fine to me. Probably advisable given there are differing opinions.

@ThYpHo0n would you be willing to amend your PR to put this behaviour behind a flag? There's already a mechanism to support this in ts-loader - it's what allows the setting of options

[johnnyreilly]

You'd need to add a new option to the LoaderOptions interface here and ensure it gets a default boolean value here.

Then you should be able to "if-else" your way to glory 😄

[ThYpHo0n]

Yes, will do so today :)

[johnnyreilly]

Awesome!

[johnnyreilly]

Hi @ThYpHo0n,

First of all, thanks for putting in the effort on the amend. I really appreciate that. It's not working at present but I imagine you know that.

However, I think you've probably taken the change in a direction I didn't expect. My understanding is that we're trying to create a flag that controls whether ts-loader logs info messages to stderr (as it does at present and has done since ts-loader 0.7) or stdout (as you and at least @SteveSandersonMS would prefer).

Your change actually introduces a log level which is not what we're trying to achieve. We just want a want to configure where info messages get sent. Would you be able to submit a change that does that instead?

(Oh and @jbrantly please do correct me if I've misunderstood anything.)

[ThYpHo0n]

Hi @johnnyreilly,
oh then I understood it the wrong way. I think that logging info messages into stdout and error messages to stderr is a good practice and would prefer this instead of a half baked solution to either write everything to stdout or stderr.

[jbrantly]

I think that logging info messages into stdout ... is a good practice

This is the part that's not universally agreed upon. In the case where the output of the application isn't just logging information but is actually some kind of real output artifact that you're going to pipe to a file or to another program, you don't want a random log to be part of that output. Here's a much deeper discussion about the issue. In actuality, I believe the concept of "any data on stderr means an error" is actually what's wrong. However, that's the world we live in unfortunately with some Microsoft tools (me included) so I think it's reasonable to say that either approach could be preferred depending on their situation.

I think which approach is default is a separate matter. My slight leaning is to keep it as-is, both because that would not introduce a breaking change and because I think there are pretty sound reasons for writing diagnostics (which is what this is) to stderr.

As an aside, what you've currently implemented with log levels also isn't a bad thing. It certainly gives more granularity than the current "silent" flag. I wonder though if "silent" should go away if log levels is implemented.

[johnnyreilly]

I think which approach is default is a separate matter. My slight leaning is to keep it as-is, both because that would not introduce a breaking change

Works for me.

As an aside, what you've currently implemented with log levels also isn't a bad thing. It certainly gives more granularity than the current "silent" flag. I wonder though if "silent" should go away if log levels is implemented.

I'm totally up for having log levels but I'd like to deal with that in a later PR if possible. Ideally I'd like to focus for now on providing a flag which controls where info messages are sent.

I'd propose calling the flag something along the lines of logInfoToStdOut (with the default being false). If true then info messages would be logged to stdout, if false then info messages would be logged to stderr. Additionally, all error messages go to stderr regardless of the setting. Does that work for everyone?

[ThYpHo0n]

Okay I understand the problem and had another discussion with my colleagues about the best practices about this topic. I also don't want to introduce BC breaking changes, the defaults should represent the current behaviour.

I have two possible solutions in my mind right now:

1. A bit dirty workaround: If silent=true I still want error logging to stderr. silent=true would then mean to solent only info logging.
2. Adding another loaderOption to determine if output to stdout is okay. I just read about adding a dash at the end of your cli command is common for it (Write compiled output to stdout microsoft/TypeScript#1226 (comment)). But I'm also totally fine with logInfoToStdOut or something similar.

[johnnyreilly]

Hi @ThYpHo0n,

Thanks for bearing with us! I'd favour having another loaderOption which controls whether info is logged to stderr or stdout explicitly I think. Explicit means it's less mysterious to users and people are less likely to forget / be surprised in future (principle of least astonishment and all that)

[johnnyreilly]

To avoid using .toUpperCase() repeatedly, could we uppercase the value when it's first read from the options in the loader function? That way we do it once rather than repeatedly (marginal perf gain) and the code reads a little cleaner (which is what I care about more).

[johnnyreilly]

Could we specify details about logInfoToStdOut before logLevel in the documentation? Also, instead of:

errors are written to stderr and everything else is written to stdout

could we say:

errors are written to stderr and everything else is written to stderr (or stdout if logInfoToStdOut is true)

[johnnyreilly]

Rather than doing .toUpperCase() repeatedly here could we initialise the value of logLevel to the uppercase value when it is first read in? That way the code will read a little cleaner and there will be a (super minor) perf gain.

[johnnyreilly]

Looks basically good - there's just a couple of points to take care of. See my comments

[johnnyreilly]

BTW - never used the "review" features in GitHub before. Hope I'm doing this right!

[ThYpHo0n]

@johnnyreilly Thanks for your feedback, will work on that later today.

[johnnyreilly]

No worries - there were 2 releases this weekend as we added support for @types and allowJs. You may find you need to refork (or maybe git will be clever - here's hoping)

[johnnyreilly]

Looks like git has not been kind 😢

Might be work re-forking and re-applying?

[ThYpHo0n]

Thats odd, the checks for the merge branch were fine. Maybe I struggle with something else there. Unfortunately the tests are broken on my machine again even being on the current origin master...

[johnnyreilly]

How are the tests failing on your machine?

[ThYpHo0n]

It gives an error on es6resolveParent test:

17 10 2016 13:18:28.797:INFO [PhantomJS 2.1.1 (Mac OS X 0.0.0)]: Connected on socket /#mZGrIr_3LdFrxyXiAAAA with id 83266213
PhantomJS 2.1.1 (Mac OS X 0.0.0) ERROR
You need to include some adapter that implements karma.start method!

[johnnyreilly]

That is really weird - I wonder if it's related to this: karma-runner/karma#1545

Looks to be a problem with using PhantomJS on Macs. Was solved. Now unsolved....

Or maybe this: karma-runner/karma-jasmine#127

EIther way - if it passes in CI that's good enough for me. This test used to work for you?

[johnnyreilly]

Looks good - tests seem happy 👍

I'll take a proper look later on but this looks nice - thanks!

[ThYpHo0n]

Great, finally good news :) Thanks for your help and the review. First time worked with TypeScript and Mocha.