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: use consolePropAttributes for k-bind properties in constructor #26850

Closed

Conversation

Projects
8 participants
@BeniCheni
Copy link
Contributor

commented Mar 22, 2019

After checking out the TODO comment in lib/internal/console/constructor.js, this PR follows the suggestion to use consolePropAttributes when defining these k-bind properties in the this object.

// TODO(joyeecheung): use consolePropAttributes for these
// Corresponds to https://console.spec.whatwg.org/#count-map
this[kCounts] = new Map();
this[kColorMode] = colorMode;
this[kIsConsole] = true;
this[kGroupIndent] = '';

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@ZYSzys

ZYSzys approved these changes Mar 23, 2019

@ZYSzys

This comment has been minimized.

@danbev

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Landed in ed5e69d.

@danbev danbev closed this Mar 27, 2019

danbev added a commit that referenced this pull request Mar 27, 2019

console: use consolePropAttributes for k-bind properties in constructor
PR-URL: #26850
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ZYSzys

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

It seems that this gone failed our travis CI (if I'm not wrong), do we need to revert this one ?

=== release test-worker-prof ===
Path: parallel/test-worker-prof
assert.js:87
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: child exited with signal: { status: null,
  signal: 'SIGSEGV',
  output: [ null, '', '' ],
  pid: 22746,
  stdout: '',
  stderr: '' }
    at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-worker-prof.js:57:10)
    at Module._compile (internal/modules/cjs/loader.js:813:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:824:10)
    at Module.load (internal/modules/cjs/loader.js:680:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:612:12)
    at Function.Module._load (internal/modules/cjs/loader.js:604:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:876:12)
    at internal/main/run_main_module.js:21:11
Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-worker-prof.js
[06:29|% 100|+ 2589|-   1]: Done
Makefile:275: recipe for target 'jstest' failed
make[1]: *** [jstest] Error 1
Makefile:291: recipe for target 'test' failed
make: *** [test] Error 2
The command "PARALLEL_ARGS='--flaky-tests=skip' make -j1 test" exited with 2.
@danbev

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I'll take a closer look now and a PR to revert this. Thanks for point this out!

danbev added a commit that referenced this pull request Mar 27, 2019

Revert "console: use consolePropAttributes for k-bind properties in c…
…onstructor"

This reverts commit ed5e69d.

PR-URL: #26943
Refs: #26850 (comment)
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@BeniCheni BeniCheni deleted the BeniCheni:console-constructor-k-bind-props branch Mar 27, 2019

@targos targos added this to Don't land (ever) in v11.x Mar 27, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 29, 2019

console: use consolePropAttributes for k-bind properties (reland)
This is a reland of nodejs#26850.
It was speculatively reverted but it turned out that this did not
cause any trouble.

PR-URL: nodejs#27352
Refs: nodejs#26943
Refs: nodejs#26850
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

targos added a commit that referenced this pull request Apr 30, 2019

console: use consolePropAttributes for k-bind properties (reland)
This is a reland of #26850.
It was speculatively reverted but it turned out that this did not
cause any trouble.

PR-URL: #27352
Refs: #26943
Refs: #26850
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.