-
Notifications
You must be signed in to change notification settings - Fork 30k
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
repl: show lexically scoped vars in tab completion #16591
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool, thank you! I can look into a PR for the stderr message if you like
lib/repl.js
Outdated
@@ -58,6 +58,7 @@ const Module = require('module'); | |||
const domain = require('domain'); | |||
const debug = util.debuglog('repl'); | |||
const errors = require('internal/errors'); | |||
const inspector = require('inspector'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this might be need to be guarded with something for --without-inspector
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lib/repl.js
Outdated
session.disconnect(); | ||
inspector.close(); | ||
return names; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait – you can use the inspector to do synchronous exception of the running process? This is absolutely amazing?? 😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's cool. However in reality it's actually a released Zalgo:
- In some cases, the callback is called synchronously.
- In others (such as
Runtime.awaitPromise
orRuntime.evaluate
withawaitPromise
option specified) the callback is called asynchronously.
I guess either nobody thought of this problem when the API or added, or synchronous calling in those circumstances under which it's possible is too valuable to force asynchrony, that it remains this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or synchronous calling in those circumstances under which it's possible is too valuable to force asynchrony
I would go with that. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, many thanks to @ofrobots who explained this to me at NINA!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conflict with existing connections is a blocker for me.
lib/repl.js
Outdated
@@ -778,6 +779,19 @@ function filteredOwnPropertyNames(obj) { | |||
return Object.getOwnPropertyNames(obj).filter(intFilter); | |||
} | |||
|
|||
function getGlobalLexicalScopeNames() { | |||
inspector.open(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inspector.open()
opens the WebSocket server, and is not needed for inspector.Session
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't know you could use the inspector without the server xD
lib/repl.js
Outdated
function getGlobalLexicalScopeNames() { | ||
inspector.open(); | ||
const session = new inspector.Session(); | ||
session.connect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will conflict with any existing open connections, e.g. when the user is experimenting with the inspector.Session
themself in the REPL. Last I heard @eugeneo was working on multi-connection Inspector support but even if it is now supported in V8, the Node.js part isn't ready yet.
> new (require('inspector').Session)().connect()
undefined
> new (require('inspector').Session)().connect()
TypeError: Session is already attached
at Session.connect (inspector.js:27:9)
at repl:1:38
lib/repl.js
Outdated
session.disconnect(); | ||
inspector.close(); | ||
return names; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's cool. However in reality it's actually a released Zalgo:
- In some cases, the callback is called synchronously.
- In others (such as
Runtime.awaitPromise
orRuntime.evaluate
withawaitPromise
option specified) the callback is called asynchronously.
I guess either nobody thought of this problem when the API or added, or synchronous calling in those circumstances under which it's possible is too valuable to force asynchrony, that it remains this way.
@addaleax Thanks, but now that I don't start the server, there is no message :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An idea I've been toying around with is always have an internal reference to the currently open inspector.Session, so that we can internally use that for side-effect-free operations like this. Doesn't have to be in this PR though.
lib/repl.js
Outdated
try { | ||
const session = new inspector.Session(); | ||
session.connect(); | ||
let names; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd initialize names
to an empty array so that if the inspector callback is actually called asynchronously things won't crash and burn.
lib/repl.js
Outdated
session.connect(); | ||
let names; | ||
session.post('Runtime.globalLexicalScopeNames', (error, result) => { | ||
names = result.names; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if !error
?
I am having trouble writing the test (see 0980529 for current state). I think the problem is that the test repl does not run in the main context, so /cc @nodejs/v8-inspector |
@@ -24,6 +24,7 @@ | |||
const common = require('../common'); | |||
const assert = require('assert'); | |||
const fixtures = require('../common/fixtures'); | |||
const hasInspector = process.config.variables.v8_enable_inspector === 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to use it when I figure out how to make the test work
Soft non-LGTM. Wouldn't this make it harder/impossible to use modules that rely on Inspector APIs in the REPL? Also, I wonder how common it is for the users to want to connect inspector to REPL node instance. Please note that https://bugs.chromium.org/p/chromium/issues/detail?id=129539 is fixed now. I would prefer to give Chrome some time to detect/fix any related issues and then add concurrent inspector sessions support to Node, probably in Node10 timeframe.
This would require cooperation from other API clients. They may rely on specific inspector session state (e.g. one client may rely on a breakpoint while another one might disable all the breakpoints for some time). Multi-session will be a cleaner solution. |
Certainly. Just wasn't sure about the timeline of such support in V8, and it could be a useful addition if such support in V8 will be a long time in coming. |
I am not exactly sure what your concern is. If an inspector session is already open, the completion should behave as it does now. If not, I connect just to get the names and disconnect immediately. The function is synchronous, so I don't see how it can affect other modules? IMO this is better than nothing, while we wait for multi-session support. |
cc @nodejs/v8-inspector again, any idea about #16591 (comment)? |
@targos You can get the However, if there are more than two contexts in play (i.e. one main, more than one REPL context), you would have to execute |
53b40e5
to
d02761d
Compare
@TimothyGu thank you, this was very informative! I think I finally got it right. CI: https://ci.nodejs.org/job/node-test-pull-request/11687/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS part LGTM
@targos this needs a rebase. And would you be so kind and start a new CI? I guess the failed tests were not an issue but it would be good to be able to look at it again. |
lib/internal/util/inspector.js
Outdated
function sendInspectorCommand(cb, onError) { | ||
if (!hasInspector) return onError(); | ||
try { | ||
const session = new inspector.Session(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of allocating a new Session on every attempt to send a command, you could allocate one Session per REPL instance to save GC some work. Just make sure to session.disconnect()
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't that make it impossible to use the inspector's JS API from the REPL? Inspector sessions are like Highlander, there can be only one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it stays disconnected between invocations of this function, I don't think keeping a Session object around is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ ./node
> const xxx = 1
undefined
> xxx # tab completion works
1
> const inspector = require('inspector')
undefined
> const session = new inspector.Session()
undefined
> session.connect()
undefined
> xx # tab completion does not work
ReferenceError: xx is not defined
> session.disconnect()
undefined
> xxx # tab completion works again
1
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu I think one session instance per process is enough. I made that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@targos it would probably be good to add the example you provided as test (while I would not test for not having the completion while a session is open but that the user session works as expected).
lib/repl.js
Outdated
@@ -158,6 +160,7 @@ function REPLServer(prompt, | |||
self.last = undefined; | |||
self.breakEvalOnSigint = !!breakEvalOnSigint; | |||
self.editorMode = false; | |||
self[kContextId] = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to clarify that this ID refers to the context ID in the inspector protocol,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d02761d
to
63fa11c
Compare
63fa11c
to
2b91ac4
Compare
Original commit message: [inspector] added Runtime.globalLexicalScopeNames method The method returns names for all available top-level scope variables in giving context. R=dgozman@chromium.org,jgruber@chromium.org Bug: chromium:681333 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I2d0b600e1afbfef9087f53ea9c26abe1e112047c Reviewed-on: https://chromium-review.googlesource.com/719409 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#48618} Refs: v8/v8@50f7455 PR-URL: #16591 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Original commit message: [inspector] added Runtime.globalLexicalScopeNames method The method returns names for all available top-level scope variables in giving context. R=dgozman@chromium.org,jgruber@chromium.org Bug: chromium:681333 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I2d0b600e1afbfef9087f53ea9c26abe1e112047c Reviewed-on: https://chromium-review.googlesource.com/719409 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#48618} Refs: v8/v8@50f7455 PR-URL: #16591 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069
This is breaking arm64 odroid on 9.x https://ci.nodejs.org/job/node-test-commit-arm/13011/nodes=ubuntu1604-arm64_odroid_c2/
I've opted to back it out of the v9.4.0 please backport if you can |
Original commit message: [inspector] added Runtime.globalLexicalScopeNames method The method returns names for all available top-level scope variables in giving context. R=dgozman@chromium.org,jgruber@chromium.org Bug: chromium:681333 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I2d0b600e1afbfef9087f53ea9c26abe1e112047c Reviewed-on: https://chromium-review.googlesource.com/719409 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#48618} Refs: v8/v8@50f7455 PR-URL: #16591 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069
Never mind... I don't think failure is related as https://ci.nodejs.org/job/node-test-commit-arm/13013/ passed with this Pr included. I've added it back to the release. |
Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069
Use the V8 inspector protocol to query the list of lexically scoped
variables (defined with
let
,const
orclass
).Fixes: #983
This is not ready to be merged. I still have to add tests and handle the case in which the inspector is already open.
Completion is working but there is one issue: every time
tab
is pressed, this message is printed to the console:Can I prevent that from happening?