no-console rule should document how to enforce calls only via no-restricted-syntax

[mockdeep]

What rule do you want to change?

The no-console rule.

Does this change cause the rule to produce more or fewer warnings?

Fewer warnings.

How will the change be implemented? (New option, new default behavior, etc.)?

I'm thinking it would be default behavior, but could also be configured.

Please provide some example code that this change will affect:

In our development and test environments we have this snippet to suss out warnings in our Javascript:

console.error = function (message) {
  throw new Error(message);
};

What does the rule currently do for this code?

The rule currently reports Unexpected console statement.

What will the rule do after it's changed?

It will not report any errors for assignment to console.

[platinumazure]

I don't think it's worth changing the rule for this use case. Instead, you can use a disable comment to ignore the rule in this location.

Here are two ways you can do this:

// eslint-disable-next-line no-console
console.error = function (message) {
  throw new Error(message);
};

// Or (equivalent)

console.error = function (message) {  // eslint-disable-line no-console
  throw new Error(message);
};

Of course, you could also just use the no-console rule to statically detect all of your console.error invocations and replace them with error throwing, as needed. 😄

[mockdeep]

Yeah, right now we're just selectively disabling the rule. In this case these are warnings given by libraries such as React, though.

[nzakas]

I'm pretty sure I wrote this rule, and the intent really was to disallow calls to console.log and friends, not to eliminate using the console identifier altogether (the docs don't mention it). As such, I think this proposal makes sense if @mockdeep is willing to implement it.

I'd suggest both changing the default and adding an option to re-enable the current behavior.

I'll champion.

[platinumazure]

@nzakas Is your proposal a breaking change?

[not-an-aardvark]

I am slightly concerned with changing the default behavior for this proposal because it would prevent code like this from being warned:

Promise.resolve()
   .then(foo)
   .then(console.log) // I put this in for debugging, but I want the rule to warn it

In other words, I would personally want the rule to warn whenever I access a property of the console object, because it's possible that another function (in this case, Promise.prototype.then) would call the method for me.

On the other hand, I suppose I could get around this by using

rules:
  no-restricted-properties: [error, {object: console}]

[mockdeep]

I'm happy to have a stab at it. Can we handle my case by just checking for assignment to it? If that is what we do, then it shouldn't change for @not-an-aardvark's case.

[platinumazure]

@kaicataldo Anything you want to add here since you had 👎'd the original post? Given your 👍 on my suggestion to use disable comments, may I assume that's your preferred alternative?

[nzakas]

@not-an-aardvark your use case wasn't intended to be supported and is not mentioned in the rule docs. You could still get that behavior with a new option.

@platinumazure Reducing the number of warnings a rule produces is not a breaking change, so it should be fine. https://github.com/eslint/eslint/blob/master/README.md#semantic-versioning-policy

@mockdeep I'd say we want a mode that operates only on CallExpressions for console.something, so basically a filter on the current default behavior. When the new option, lets call it checkAllReferences:true (false by default), it should just not apply that filter.

[not-an-aardvark]

This has 3 👍s and a champion, so I'm marking it as accepted. I added the "help wanted" label for now, but if anyone on the team wants to work on it, feel free to remove the label.

[wincent]

Another use case: we do this kind of thing in tests like this one:

expect(console.warn).not.to.have.been.called();

For now, just suppressing with eslint-disable no-console comment in the test file.

[adventure-yunfei]

I'm trying to help this issue, but I found it may not make sense.

When we override console.log or something similar, we normally injected custom behavior based on existed behavior. Example code like this:

var originalLog = console.log;
console.log = function (msg) {
    // Custom logic for "msg"
    // Then keep original behavior
    originLog.call(this, msg);
};

Even we add a new option not to warning for the re-assignment, we still get a warning on the previous var originalLog = console.log;.

[aladdin-add]

working on this.

[kaicataldo]

Removed my 👎. I'm not strongly opposed to this, and it sounds like the rest of the team is on board.

[platinumazure]

Some questions were raised about whether ignoring assignment is the best way to handle this. Can I withdraw my 👍 (for now) and we can continue discussing whether there's anything we can sanely do for this rule?

In particular, I'm wondering if no-restricted-syntax could be used for the case of wanting to only forbid calls to the console functions (using a selector like CallExpression[callee.object.name='console'][callee.property.name=/^(log|warn|error|info|trace)$/]).

[not-an-aardvark]

Personally, I think ignoring assignments is fine here since it seems like that was the original intention of the rule. It's true that no-restricted-syntax could be used to only forbid calls, but I think no-restricted-syntax could be used to imitate all the behaviors that we're discussing here anyway. So I don't think redundancy with no-restricted-syntax should be something that affects the decision here.

[not-an-aardvark]

Adding evaluating to this issue because @platinumazure's 👍 was withdrawn in #7806 (comment). I'm also adding this to the TSC agenda since we haven't been able to reach consensus in this issue, although if we reach consensus here before the TSC meeting I can remove it again.

TSC Summary: This issue proposes making no-console less strict by only having it report calls to console methods, rather than reporting any property access on console.
TSC Question: Should we accept this issue?

[btmills]

In today's meeting, the TSC decided against implementing this as an option in the rule, preferring to keep the current behavior. We would, however, like to add a note to the no-console docs showing how to accomplish the call-only behavior with no-restricted-syntax as described by @platinumazure.

Marking this as an accepted documentation change.

[platinumazure]

Editing the title of this issue to reflect the TSC decision.