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

commented Nov 28, 2018

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
joyeecheung added 3 commits Nov 28, 2018
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.
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.

@joyeecheung joyeecheung requested a review from devsnek Nov 28, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

@@ -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;

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 28, 2018

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@joyeecheung

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

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

@addaleax
Copy link
Member

left a comment

More or less rubber-stamp LGTM

@jasnell
jasnell approved these changes Dec 1, 2018
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

Landed in adbf947...edb8f22, thanks!

@joyeecheung joyeecheung closed this Dec 1, 2018

joyeecheung added a commit that referenced this pull request Dec 1, 2018
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.

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

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

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

This comment has been minimized.

Copy link
Member

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.

@targos targos added this to Backport requested in v11.x Dec 7, 2018

BridgeAR added a commit that referenced this pull request Jan 10, 2019
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.

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 added a commit that referenced this pull request Jan 10, 2019
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.

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 added a commit that referenced this pull request Jan 10, 2019
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.

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

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 10, 2019

addaleax added a commit that referenced this pull request Jan 14, 2019
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.

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 added a commit that referenced this pull request Jan 14, 2019
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.

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 added a commit that referenced this pull request Jan 14, 2019
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.

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 added a commit to refack/node that referenced this pull request Jan 14, 2019
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.

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 added a commit to refack/node that referenced this pull request Jan 14, 2019
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.

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 added a commit to refack/node that referenced this pull request Jan 14, 2019
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.

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 referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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.

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 added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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.

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 added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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.

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 referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants
You can’t perform that action at this time.