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

Possibly unnecessary code? #4581

Closed
zeusdeux opened this issue Jan 8, 2016 · 6 comments
Closed

Possibly unnecessary code? #4581

zeusdeux opened this issue Jan 8, 2016 · 6 comments
Labels
console Issues and PRs related to the console subsystem.

Comments

@zeusdeux
Copy link
Contributor

zeusdeux commented Jan 8, 2016

While reading the lib/console.js source to triage another issue, I noticed that I didn't understand the need for these lines of code. What is the rationale behind binding prototype methods to the new this and then sticking 'em on the new object being created?

The only places removing these lines has an affect is in:

  1. parallel/test-console-instance.js where on line 58 we can just change it to c.log.bind(c) to fix the test
  2. TickProcessor.printStatistics definition which is eval-d as a part of the test-tick-processor.js test

I tried to pin point what the issue was but I wasn't able to use require, console, etc in v8_prof_processor.js which is called as a part of the test-tick-processor.js test and that left me kind of handicapped.

@targos
Copy link
Member

targos commented Jan 8, 2016

It was probably done for convenience. By binding the methods to instance, they can easily be passed as callbacks:

fs.stat('foo.txt', console.log);
Promise.resolve('foo').then(console.log)

@targos targos added the console Issues and PRs related to the console subsystem. label Jan 8, 2016
@zeusdeux
Copy link
Contributor Author

zeusdeux commented Jan 8, 2016

@targos But that's not how browsers do it though. Is the convenience and probably some kind of perf hit worth the disparate semantics from browserland?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 8, 2016

It looks like what @targos said was the reasoning in the original commit - 025f53c. Node's console makes no guarantees about compatibility with browsers, and at this point, it would be an unnecessary breaking change.

@zeusdeux
Copy link
Contributor Author

zeusdeux commented Jan 8, 2016

With all the isomorphic javascript projects showing up, console behaviour parity between node and browsers might be useful. I can imagine isomorphic code having redundant .bind(console) or having that logic behind guards checking for node or browser. Seems superfluous.

Also, it exists in node as console mainly because JS devs are used to using console in the browser. So that to me justifies behaviour parity/compatibility.

@sindresorhus
Copy link

There's a ton of code using console.log statically in the Node.js ecosystem. Not worth breaking. I think browsers should change instead. Though, that's better discussed here: whatwg/console#3

@zeusdeux
Copy link
Contributor Author

I don't think browsers should change (since it changes this semantics, etc) but I see the point wrt the node ecosystem.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants