console: minor console refactoring #17707

BridgeAR · 2017-12-16T07:26:36Z

Some minor changes

console: make error handling engine agnostic

Calling write could throw a maximum call stack size error. To make
sure this is not specific to a single engine (version), lazily
populate the correct error message by producing such a error on
demand.

console: make variables and checks stricter

console: use spread instead of apply

The performance seems pretty similar to the any other method and
this is the most readable one.
Also fix and refactor the console benchmark. It did not test console
so it got renamed and mainly tests the different ways of passing
arguments through

console: order functions and remove \n\n

This lists all function aliases directly below the declared function.

doc: remove old console note

This should not be important anymore in newer docs. The change is
also documented in the "changed" part of the function.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

console, doc, benchmark

Calling write could throw a maximum call stack size error. To make sure this is not specific to a single engine (version), lazily populate the correct error message by producing such a error on demand.

starkwang · 2017-12-16T13:29:59Z

lib/console.js

@@ -50,7 +52,7 @@ function Console(stdout, stderr, ignoreErrors = true) {
  Object.defineProperty(this, '_stdout', prop);
  prop.value = stderr;
  Object.defineProperty(this, '_stderr', prop);
-  prop.value = ignoreErrors;
+  prop.value = !!ignoreErrors;


Using Boolean(ignoreErrors) might be more readable here.

bnoordhuis · 2017-12-16T17:00:48Z

lib/console.js


 Console.prototype.warn = function warn(...args) {
  write(this._ignoreErrors,
        this._stderr,
-        util.format.apply(null, args),
+        util.format(...args),


The spread operator, unlike .apply(), pushes the elements onto the stack. That is, it makes stack overflows more likely.

Fix and refactor the console benchmark. It did not test console so it got renamed and mainly tests the different ways of passing arguments through.

This lists all function aliases directly below the declared function.

This should not be important anymore in newer docs. The change is also documented in the "changed" part of the function.

BridgeAR · 2017-12-16T18:54:14Z

Comments addressed. I removed the spread operator again and added a comment about it.

lpinca · 2017-12-17T21:03:43Z

benchmark/misc/arguments.js

+  n: [1e6]
+});
+
+const nullStream = createNullStream();


Unrelated but can't this be simplified to just new Writable({ write: () => {} })?

I went ahead and removed the fake console overall as it had no real purpose in this benchmark.

BridgeAR · 2017-12-19T02:35:17Z

CI https://ci.nodejs.org/job/node-test-pull-request/12189/

BridgeAR · 2017-12-19T16:01:06Z

New CI https://ci.nodejs.org/job/node-test-pull-request/12201/

BridgeAR · 2017-12-20T04:35:44Z

Landed in 1e84e69, 5b4d532, 0eab495, 1d6b729, 5198a53

Calling write could throw a maximum call stack size error. To make sure this is not specific to a single engine (version), lazily populate the correct error message by producing such a error on demand. PR-URL: nodejs#17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix and refactor the console benchmark. It did not test console so it got renamed and mainly tests the different ways of passing arguments through. PR-URL: nodejs#17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This lists all function aliases directly below the declared function. PR-URL: nodejs#17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This should not be important anymore in newer docs. The change is also documented in the "changed" part of the function. PR-URL: nodejs#17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Calling write could throw a maximum call stack size error. To make sure this is not specific to a single engine (version), lazily populate the correct error message by producing such a error on demand. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix and refactor the console benchmark. It did not test console so it got renamed and mainly tests the different ways of passing arguments through. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This lists all function aliases directly below the declared function. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This should not be important anymore in newer docs. The change is also documented in the "changed" part of the function. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Calling write could throw a maximum call stack size error. To make sure this is not specific to a single engine (version), lazily populate the correct error message by producing such a error on demand. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix and refactor the console benchmark. It did not test console so it got renamed and mainly tests the different ways of passing arguments through. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This lists all function aliases directly below the declared function. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This should not be important anymore in newer docs. The change is also documented in the "changed" part of the function. PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-23T08:14:21Z

should this be backported to v6.x or v8.x?

MylesBorins · 2018-05-22T17:43:34Z

@BridgeAR should this be backported?

console: make error handling engine agnostic

26bf70e

Calling write could throw a maximum call stack size error. To make sure this is not specific to a single engine (version), lazily populate the correct error message by producing such a error on demand.

nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Dec 16, 2017

starkwang reviewed Dec 16, 2017

View reviewed changes

bnoordhuis reviewed Dec 16, 2017

View reviewed changes

BridgeAR force-pushed the refactor-console branch from bc57bcb to ce00d93 Compare December 16, 2017 18:48

console: make variables and checks stricter

6145af1

BridgeAR force-pushed the refactor-console branch from ce00d93 to 74755f0 Compare December 16, 2017 18:49

BridgeAR added 3 commits December 16, 2017 16:53

benchmark: refactor console benchmark

c511a5d

Fix and refactor the console benchmark. It did not test console so it got renamed and mainly tests the different ways of passing arguments through.

console: order functions and remove \n\n

a4e3aa5

This lists all function aliases directly below the declared function.

doc: remove old console note

2dc5813

This should not be important anymore in newer docs. The change is also documented in the "changed" part of the function.

BridgeAR force-pushed the refactor-console branch from 74755f0 to 2dc5813 Compare December 16, 2017 18:54

lpinca approved these changes Dec 17, 2017

View reviewed changes

fixup - address comment

7f65b0b

jasnell approved these changes Dec 18, 2017

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2017

BridgeAR closed this Dec 20, 2017

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 20, 2017

console: make variables and checks stricter

1d6b729

PR-URL: nodejs#17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins pushed a commit that referenced this pull request Jan 8, 2018

console: make variables and checks stricter

820e978

PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Jan 9, 2018

console: make variables and checks stricter

41e2bb1

PR-URL: #17707 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 23, 2018

This comment has been minimized.

Sign in to view

MylesBorins added backport-requested-v8.x and removed lts-watch-v6.x labels May 22, 2018

BridgeAR deleted the refactor-console branch April 1, 2019 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console: minor console refactoring #17707

console: minor console refactoring #17707

BridgeAR commented Dec 16, 2017

starkwang Dec 16, 2017

bnoordhuis Dec 16, 2017

BridgeAR commented Dec 16, 2017

lpinca Dec 17, 2017

BridgeAR Dec 18, 2017 •

edited

Loading

BridgeAR commented Dec 19, 2017

BridgeAR commented Dec 19, 2017

BridgeAR commented Dec 20, 2017

MylesBorins commented Jan 23, 2018

This comment has been minimized.

MylesBorins commented May 22, 2018

console: minor console refactoring #17707

console: minor console refactoring #17707

Conversation

BridgeAR commented Dec 16, 2017

Checklist

Affected core subsystem(s)

starkwang Dec 16, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 16, 2017

Choose a reason for hiding this comment

BridgeAR commented Dec 16, 2017

lpinca Dec 17, 2017

Choose a reason for hiding this comment

BridgeAR Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Dec 19, 2017

BridgeAR commented Dec 19, 2017

BridgeAR commented Dec 20, 2017

MylesBorins commented Jan 23, 2018

This comment has been minimized.

MylesBorins commented May 22, 2018

BridgeAR Dec 18, 2017 •

edited

Loading