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

Console.log bug? #23137

Closed
dtroydev opened this issue Sep 28, 2018 · 11 comments
Closed

Console.log bug? #23137

dtroydev opened this issue Sep 28, 2018 · 11 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. console Issues and PRs related to the console subsystem.

Comments

@dtroydev
Copy link

  • Version: v10.6.0
  • Platform: Mac OS 10.13.6
  • Subsystem: console

Suppose we used console.log as follows:

console.log('I\'m trying to escape', false);
console.log(false, 'I\'m trying to escape');

This will then output the following:

I'm trying to escape false
false 'I\'m trying to escape'

As can be seen the backslash is treated differently between lines.

Also in the actual shell, the colouring is not present in the first line, but is present in the second line.
(boolean is yellow and string is green)

Is this by design or a bug?

@addaleax addaleax added question Issues that look for answers. console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. and removed question Issues that look for answers. labels Sep 28, 2018
@addaleax
Copy link
Member

Is this by design or a bug?

I agree that it’s super confusing, but it’s by design. If the first argument is a string, it is used as a format string – the console.log() documentation isn’t great on explaining this, but it links to util.format(). There, we find this sentence:

the first argument is not a string then util.format() returns a string that is the concatenation of all arguments separated by spaces. Each argument is converted to a string using util.inspect().

I really think we should improve the console.log documentation here, though.

@addaleax
Copy link
Member

I’m reopening this so that we can keep it as a todo for the documentation, hope that’s okay.

@addaleax addaleax reopened this Sep 28, 2018
@lovinglyy
Copy link
Contributor

I would love to do this one as good first issue, I do have a contribution already but not that relevant, is it okay?

@lundibundi
Copy link
Member

@lovinglyy Yes, go on. 👍

If you have any questions you can post them here.

@silverwind
Copy link
Contributor

silverwind commented Sep 28, 2018

I think our behaviour might not be conforming the Console Standard. If I interpret the spec correctly, we should arrive here with logLevel being 'log' and there is no mentioning that the first argument is to be treated differently from the others, or am I missing something?

@devsnek
Copy link
Member

devsnek commented Sep 28, 2018

@silverwind
Copy link
Contributor

silverwind commented Sep 28, 2018

@devsnek I don't think so. It should branch to Printer on step 5 of Logger when the first argument does not contain a format specifier (e.g. %d):

If first does not contain any format specifiers, perform Printer(logLevel, args).

@lovinglyy
Copy link
Contributor

Should I wait to see if there will be changes in console.log behavior before doing pull requests to the docs?

@silverwind silverwind removed the good first issue Issues that are suitable for first-time contributors. label Sep 29, 2018
@silverwind
Copy link
Contributor

silverwind commented Sep 29, 2018

Working on a fix, not sure if it's feasible but it may just be, so we won't have to document this broken behaviour.

@silverwind silverwind self-assigned this Sep 29, 2018
@silverwind silverwind added confirmed-bug Issues with confirmed bugs. and removed doc Issues and PRs related to the documentations. labels Sep 29, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Oct 5, 2018

I just checked the spec and to me it leaves a lot of interpretation to the implementer.

The console.log function is an equivalent to Logger('log', data) in the spec. So the Logger itself will check if the first argument contains format specifiers or not. If it does not, it triggers the Printer. The printer definition is very vague and almost everything is open to the implementer in this case.
If the first argument does contain format specifiers, it will call Formatter. The formatter should always return a list which corresponds to an Array in JS. So we actually do not adhere to the spec with util.format() since it returns a string in Node.js. That is somewhat ironic since the spec has an example using Node.js and util.format(). However, we could call this an implementation detail since the formatter function is meant to be a helper function that is not meant to be exposed by the spec.

Looking at the implementations in browsers it seems like Chrome behaves identical to Node.js and Firefox always formats the output.
Both behaviors seem aligned by the spec as the printer function does not specify how the arguments should be visualized:

How the implementation prints args is up to the implementation, but implementations should separate the objects by a space or something similar, as that has become a developer expectation.

Due to this, I would say we can implement this in what ever way we want.

@silverwind
Copy link
Contributor

Yes, our code does not strictly implement the abstractions like Formatter from the spec, but that's something that could be solved with a bit of refactoring. The primary issue is thought that util.format is exposed to user code, so we obviously cannot change its return type.

I actually prefer Firefox's formatting to Chrome/Node, but I don't think this is something that can be changed without considerable breakage.

silverwind added a commit to silverwind/node that referenced this issue Oct 16, 2018
Two changes here which bring us closer to the console standard:

- Arguments to `util.format` are no longer formatted differently
  depending on their order, with format strings being an exception.
- Format specifier formatting is now only triggered if the string
  actually contains a format string.

Under the hood, we now use a single shared function to format the given
arguments which will make the code easier to read and modify.

PR-URL: nodejs#23162
Fixes: nodejs#23137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. console Issues and PRs related to the console subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants