★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/26850Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
wants to merge 1 commit into from

Conversation

Reviewers

@joyeecheung joyeecheung

@BridgeAR BridgeAR

@ZYSzys ZYSzys

@mihalskiy mihalskiy

Assignees
No one assigned
Labels
author ready console dont-land-on-v10.x dont-land-on-v8.x
Projects
  
Don't land (ever) in v11.x
Milestone
No milestone
8 participants
@BeniCheni @ZYSzys @danbev @joyeecheung @BridgeAR @mihalskiy @targos @nodejs-github-bot
@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.

node/lib/internal/console/constructor.js

Lines 184 to 189 in db40b4a

// 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

@nodejs-github-bot nodejs-github-bot added the console label Mar 22, 2019

@mihalskiy

mihalskiy approved these changes Mar 22, 2019
View changes

@joyeecheung

joyeecheung approved these changes Mar 23, 2019
View changes

@ZYSzys

ZYSzys approved these changes Mar 23, 2019
View changes

@ZYSzys

This comment has been minimized.

Sign in to view
Copy link
Member

commented Mar 23, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/21826/
@BridgeAR

BridgeAR approved these changes Mar 25, 2019
View changes

@BridgeAR BridgeAR added the author ready label Mar 25, 2019

@danbev

This comment has been minimized.

Sign in to view
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

@BeniCheni @danbev
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>
Loading status checks…
ed5e69d
@ZYSzys

This comment has been minimized.

Sign in to view
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.

Sign in to view
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 danbev referenced this pull request Mar 27, 2019

Closed

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

2 of 2 tasks complete

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

@danbev
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>
Loading status checks…
a8eac78

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

@targos targos added dont-land-on-v6.x dont-land-on-v8.x dont-land-on-v10.x labels Mar 27, 2019

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

This was referenced Apr 23, 2019

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed

@beevelop beevelop referenced this pull request Apr 23, 2019

Closed

2019-04-23 Version 12.0.0 (Current) @BethGriggs #306

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

@BridgeAR
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
6bfde6a

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

@BridgeAR @targos
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
8e876e6
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.