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: split console.js into constructor.js, global.js and inspector.js for clarity #24709

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

This PR may be easier to review commit-by-commit. I tried some git trick so that the majority of the history of the console implementation is still available in git blame.

lib: move lib/console.js to lib/internal/console/constructor.js

This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor in git blame.

console: split console into global.js and constructor.js

Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.

console: move the inspector console wrapping in a separate file

Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
require('internal/console/inspector').consoleFromVM
that require('inspector').console can alias to it later,
instead of hanging the original console onto per_thread.js
during bootstrap and counting on that per_thread.js
only gets evaluated once and gets cached.

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

This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor.
Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.
Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
`require('internal/console/inspector').consoleFromVM`
that `require('inspector').console` can alias to it later,
instead of hanging the original console onto `per_thread.js`
during bootstrap and counting on that `per_thread.js`
only gets evaluated once and gets cached.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 28, 2018
@joyeecheung
Copy link
Member Author

@@ -9,7 +9,7 @@ const common = require('../common');
const assert = require('assert');

const isMainThread = common.isMainThread;
const kMaxModuleCount = isMainThread ? 56 : 78;
const kMaxModuleCount = isMainThread ? 58 : 80;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increases the number of module being loaded during bootstrap a little bit, but my hope is to resolve the TODO I added in setupGlobalConsole later so that we don't wrap the console with inspector APIs when inspector is not even activated (I believe it used to be that way, but somehow the lazy-loading got lost during refactoring? cc @eugeneo ).

@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added process Issues and PRs related to the process subsystem. console Issues and PRs related to the console subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Nov 30, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 30, 2018

@Trott
Copy link
Member

Trott commented Dec 1, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19107/ ✔️

@Trott
Copy link
Member

Trott commented Dec 1, 2018

Could use a second review. Maybe @addaleax @BridgeAR @jasnell ?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More or less rubber-stamp LGTM

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2018
@joyeecheung
Copy link
Member Author

Landed in adbf947...edb8f22, thanks!

@joyeecheung joyeecheung closed this Dec 1, 2018
joyeecheung added a commit that referenced this pull request Dec 1, 2018
This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Dec 1, 2018
Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Dec 1, 2018
Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
`require('internal/console/inspector').consoleFromVM`
that `require('inspector').console` can alias to it later,
instead of hanging the original console onto `per_thread.js`
during bootstrap and counting on that `per_thread.js`
only gets evaluated once and gets cached.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Dec 5, 2018

This does not land cleanly on 11.x (this relies on a former PR which does not land cleanly. As soon as that landed it should likely land properly). Please open a manual backport or change the label accordingly.

BridgeAR pushed a commit that referenced this pull request Jan 10, 2019
This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jan 10, 2019
Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jan 10, 2019
Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
`require('internal/console/inspector').consoleFromVM`
that `require('inspector').console` can alias to it later,
instead of hanging the original console onto `per_thread.js`
during bootstrap and counting on that `per_thread.js`
only gets evaluated once and gets cached.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

I backported this directly on the staging branch as there were only few conflicts left with the recent console commits landed.

addaleax pushed a commit that referenced this pull request Jan 14, 2019
This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
`require('internal/console/inspector').consoleFromVM`
that `require('inspector').console` can alias to it later,
instead of hanging the original console onto `per_thread.js`
during bootstrap and counting on that `per_thread.js`
only gets evaluated once and gets cached.

PR-URL: #24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor.

PR-URL: nodejs#24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.

PR-URL: nodejs#24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
`require('internal/console/inspector').consoleFromVM`
that `require('inspector').console` can alias to it later,
instead of hanging the original console onto `per_thread.js`
during bootstrap and counting on that `per_thread.js`
only gets evaluated once and gets cached.

PR-URL: nodejs#24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
This is a broken commit: it's here so that git interpret this
as a file move and preserve most of the history of the Console
constructor.

PR-URL: nodejs#24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Since we do not actually use the Console constructor to
instantiate the global console, move the two piece of
code into two different JS files for clarity, and make
console.js a mere re-export of the global console.
The hope is to make the global console, a namespace, more
web-compatible while keeping the Console constructor
available for backwards compatibility.

PR-URL: nodejs#24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Move the wrapping of the inspector console in a separate file
for clarity. In addition, save the original console from the
VM explicitly via an exported property
`require('internal/console/inspector').consoleFromVM`
that `require('inspector').console` can alias to it later,
instead of hanging the original console onto `per_thread.js`
during bootstrap and counting on that `per_thread.js`
only gets evaluated once and gets cached.

PR-URL: nodejs#24709
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants