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: refactor inspector console extension installation #25450

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 11, 2019

  • Instead of creating the console extensions eagerly during bootstrap
    and storing them on an object, wrap the code into a function to be
    called during installAdditionalCommandLineAPI only when the extensions
    are actually needed, which may not even happen if the user do not
    use the console in an inspector session, and does not need to happen
    during bootstrap unconditionally.
  • Simplify the console methods wrapping and move the consoleFromVM
    storage to internal/util/inspector.js
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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 11, 2019
Copy link
Contributor

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

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

This change messes the top stack frame - instead of displaying user code location it returns internal/util/inspector.js:42

This is unacceptable as some popular tools (e.g. Chrome DevTools Console) only display that location and open it in an editor when the user clicks it.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 11, 2019

This change messes the top stack frame - instead of displaying user code location it returns internal/util/inspector.js:42

Is it possible to write a test for this? It seems pretty fragile to have this contract depended on by external tools without any tests.

Also, is it talking about the error stacks or the stack frames returned through the inspector protocol?

@eugeneo
Copy link
Contributor

eugeneo commented Jan 11, 2019

I created a new pull request with a test: #25455

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jan 12, 2019
Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

In addition to the issue @eugeneo highlighted with the call stack, removing .consoleCall from inspector binding makes it impossible to customize console methods for inspector use. We should expose consoleCall in a similar way to inspector.console.

@joyeecheung joyeecheung added inspector Issues and PRs related to the V8 inspector protocol console Issues and PRs related to the console subsystem. and removed blocked PRs that are blocked by other issues or PRs. labels Jan 16, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 16, 2019

Rebased after #25455 and removed consoleCall changes. @eugeneo @jdalton PTAL.

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

- Instead of creating the console extensions eagerly during bootstrap
  and storing them on an object, wrap the code into a function to be
  called during `installAdditionalCommandLineAPI` only when the extensions
  are actually needed, which may not even happen if the user do not
  use the console in an inspector session, and does not need to happen
  during bootstrap unconditionally.
- Simplify the console methods wrapping and move the `consoleFromVM`
  storage to `internal/util/inspector.js`
consoleAPIModule.paths =
CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths);
commandLineApi.require = makeRequireFunction(consoleAPIModule);
}
Copy link
Member

Choose a reason for hiding this comment

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

👆 Could someone explain this bit. I'm unfamiliar with the '<inspector console>' and commandLineApi.require bits. Is there a require exposed in the Chrome inspector console?

Copy link
Member

@devsnek devsnek Jan 16, 2019

Choose a reason for hiding this comment

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

The inspector console is the one in the chrome inspector. By doing this, a require is exposed when you connect a chrome inspector to node. (And Runtime.evaluate if you enable it)

Copy link
Member

@jdalton jdalton Jan 16, 2019

Choose a reason for hiding this comment

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

Is this something user accessible? If so, what does accessing it look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdalton: it's accessed when using require in DevTools console.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 18, 2019

Ping @eugeneo I've removed the consoleCall changes, can you take a look again? I'd like to land this so that 'internal/modules/cjs/loader' does not have to be loaded this early during bootstrap so that we can split the bootstrapping process from the execution mode selection which helps the snapshot effort.

@joyeecheung
Copy link
Member Author

Ping @eugeneo

@joyeecheung
Copy link
Member Author

I am going to dismiss #25450 (review) since it's been addressed a week ago without response, and it no longer applies to this PR since this passes the test added in #25455

@joyeecheung
Copy link
Member Author

function sendInspectorCommand(cb, onError) {
const { hasInspector } = internalBinding('config');
const inspector = hasInspector ? require('inspector') : undefined;
if (!hasInspector) return onError();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe just switch around the above two lines? That way there's no need for the ternary conditional.

const inspector =
NativeModule.require('internal/console/inspector');
inspector.addInspectorApis(consoleFromNode, consoleFromVM);
const inspector = NativeModule.require('internal/util/inspector');
// This will be exposed by `require('inspector').console` later.
inspector.consoleFromVM = consoleFromVM;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to move this into internal/util/inspector. That way there's no need for the getter on module.exports.

Copy link
Member Author

@joyeecheung joyeecheung Jan 23, 2019

Choose a reason for hiding this comment

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

The console from the VM is only accessible during bootstrap, global.console gets overridden with our console implementation - an alternative may be to store it in C++ and make it accessible (and cached) through 'internal/util/inspector', but otherwise this has to be done in lib/internal/bootstrap/node.js one way or another (stored either as a value or through a setter)

Copy link
Member

Choose a reason for hiding this comment

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

consoleFromVM is passed to inspector.wrapConsole() in the line below. Therefore the consoleFromVM variable in lib/internal/util/inspector.js could be set during the wrapConsole function call.

But it's just a nit and is not blocking for me.

@joyeecheung
Copy link
Member Author

joyeecheung added a commit that referenced this pull request Jan 23, 2019
- Instead of creating the console extensions eagerly during bootstrap
  and storing them on an object, wrap the code into a function to be
  called during `installAdditionalCommandLineAPI` only when the
  extensions are actually needed, which may not even happen if the
  user do not use the console in an inspector session, and does not
  need to happen during bootstrap unconditionally.
- Simplify the console methods wrapping and move the `consoleFromVM`
  storage to `internal/util/inspector.js`

PR-URL: #25450
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@joyeecheung
Copy link
Member Author

Landed in d3806f9

addaleax pushed a commit that referenced this pull request Jan 23, 2019
- Instead of creating the console extensions eagerly during bootstrap
  and storing them on an object, wrap the code into a function to be
  called during `installAdditionalCommandLineAPI` only when the
  extensions are actually needed, which may not even happen if the
  user do not use the console in an inspector session, and does not
  need to happen during bootstrap unconditionally.
- Simplify the console methods wrapping and move the `consoleFromVM`
  storage to `internal/util/inspector.js`

PR-URL: #25450
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants