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
Node.js' internal scripts should be hidden in the inspector. #11893
Comments
@hashseed latter was marked as wontfix a while ago: https://bugs.chromium.org/p/v8/issues/detail?id=1574 |
@hashseed - while (live) debugging a problem at hand, what would be the chances that the user want to focus only on his code? I guess an average program will be a blend of API invocations, thirdparty modules and the user's own code, and the scope of the investigation could span across and thereby equally applicable to all? When you say blackboxing code regions, do you mean not to provide debug control over those regions, or hiding them from the view alone, or both? |
I would at least like to still be able to set some option to expose core code for... core debugging purposes. |
I use inspector personally to help debug core stuff from time to time. Hiding these would not be ideal. Perhaps if there was a better way of organizing them, that would be better. |
Blackboxing by default would still allow the blackbox pattern to be changed to reveal internal scripts. |
That's the |
I haven't verified, but the command seems correct. Internal scripts have the same script type as normal ones, so I guess by filename alone makes it not fool proof. But maybe having special file names, e.g. node-internal://module.js is sufficient? |
I think blackboxing by default is probably enough(it's what people do when writing frontend code with jQuery and stuff), even users can track down a core bug with it if they have the time & patience, that would be very useful for bug reports. |
This should remain open? |
I think so
…On Jul 29, 2017 11:32 PM, "Rich Trott" ***@***.***> wrote:
This should remain open?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11893 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOUo0zppPdp4ijs_p6nkdyxjC23VdrWks5sTAdHgaJpZM4MgSQm>
.
|
@eugeneo @sam-github @jkrems I'd be happy to tackle this, but would appreciate a starting point. |
I'm not sure I'm a fan of hiding them. I personally treat those scripts just like any other "3rd party" code. It seems fairly arbitrary to hide As for helpful reactions: I don't think I was even aware of |
I'd be happy with blackboxing the Node.js internal scripts by default but would definitely want the option of switching that off by default. |
@hashseed Is the Inspector command the only way to get to this feature? Is it possible to do this from C++'s |
There hasn't seen a comment on this in over a year, so I'm going to close this. Feel free to re-open if it's still something that we want to do. Or (probably better) leave it closed but add it to the feature requests project if it's unlikely to have anyone working on it soon. FWIW, I'm with @jkrems on this one: Unless there's evidence that this is causing confusion or difficulty for end users, I'd prefer we expose core code in the debugger rather than blackbox it. I suspect (but don't know) that @addaleax might feel the same. |
I think blackboxing them by default would be better than hiding them outright - it's probably orthogonal to the coverage if the only behavior change is being hidden by default in the DevTools UI. I am reopening this. Happy to take look at this now that the builtin modules are all compiled from C++ in one central API. |
I gave a try at this in https://github.com/joyeecheung/node/tree/blackbox by prefixing the resource names of internal scripts with
(Personally this looks odd/inconsistent to me unless we also prefix the file names with It is possible to fix up the resource names and restore them to what they use to be when producing the error stack traces, but I'd prefer not to hack on the current code but build on some refactoring like #23926 So my current plan is to see if I can help making any progress on #23926 first. |
I like the |
@piranna If you are talking about ESM import, those are not related to this issue - this is only for core modules which are not ESM, and Node core does not support loading modules from remote either (without custom loaders) so I think
What do you mean by |
Yes, I was talking about ESM modules, how are they shown when they are imported from a remote location? Using a scheme? By default I means until ESM all modules has been get from the local filesystem, so I find it like a sane default... |
@piranna That's separate from this thread, core modules are not compiled in the same way as user land modules, and they are not ESM.
I don't think the default resides in the URL protocol? If we were to add protocols and make the resource names proper URLs, that protocol should come from the module loader and reflect where the source comes from, not from the compiler - this thread is talking specifically about the compiler of core CJS modules, whose source are built into the binary and are not loaded from anywhere external. |
Can we keep |
If we expose it in the stack trace, should we call it |
Oh, I do agree if that's a thing now we should follow the same pattern and have |
@mmarchini after a few minor back and forths policies and import have converged recently, docs just landed in #35434 |
oh that's |
Yeah, historically // is signalling that a sche has relative url strings
resolve against it ( ./ , ../ , etc. ). Only file: and http(s): do that
still in the WHATWG sense
…On Fri, Oct 2, 2020, 4:10 PM mary marchini ***@***.***> wrote:
oh that's node:module and not node://module, but in the end it doesn't
matter as we can use it as well here :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11893 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABZJI574VJI2WIAQELE4VTSIY6TXANCNFSM4DEBEQTA>
.
|
So would it be acceptable to make the builtin modules compiled with |
OffTopic:
And in fact, Tim Berners Lee told in an interview that the Take in mind we are talking early 90's, where Gopher and BBSs were the standard hypertext protocol at that time and there were no web search engines, so maybe he was thinking about agregating domains on topics like news, games, software, xxx... similar to how Usenet categories are organized :-) That's why for |
@joyeecheung it's acceptable IMO. It will also make parsing stack traces (both on errors and profiles) easier to find Node.js built-in modules. |
This change allows for easier recognition of builtin modules in stack traces. Refs: nodejs#11893
PR at #35498 |
This change allows for easier recognition of builtin modules in stack traces. Refs: #11893 PR-URL: #35498 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>
should this remain open? I see the linked PR (for qualifying core modules) is landed, but not sure if it fully addresses the original proposal (hiding core modules from inspector). |
Maybe something can be done from DevTool's side to hide these scripts by default? Though I am not sure if that's desired behavior. We should post something to teach users how to make use of this feature to blackbox the internal scripts, though. |
Can this be implemented on Node.js by running an inspector command to blackbox internal scripts when DevTools connects to the agent? |
Maybe, we can send
to the inspector, though we also need to send |
We had a call to volunteers in the diag meeting today, so I'll remove from the agenda. |
Hey! I've taken a look at it today and I wondering if it should be done either on the Node.js or Dev tools. Based on the message above seems that we just need to send this message through WebSocket on the debugger startup. Besides sending the pattern via DevTools, would be good to change it in the inspector as well, I think here specifically. Not sure if I understood correctly, but if yes, I can help as I can on |
This change allows for easier recognition of builtin modules in stack traces. Refs: nodejs#11893 PR-URL: nodejs#35498 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>
It's been a few years since any activity on this issue, but I'd like to give my opinion given the amount of input it's received. I'm firmly against this, as the internal scripts are extremely helpful in the inspector. When debugging a new feature (or fixing an old one) in Node.js, I find it very helpful to see where in the internal systems everything is happening. From a triage perspective, there hasn't been any activity on this issue in a while, and if it's not going to be pursued, please close it. Nonetheless, your efforts are appreciated, and we are happy to review future issues :-). |
Personally I would still be happy to see this option implemented in the inspector clients (well, the sever can send the commands if necessary) especially for the async hooks - since DevTools sets maximum async stack depth by default (maybe making that optional would be another great feature), not black boxing the internal scripts means that step-debugging every Promise or async/await would get you tripped on the promise hooks several times and it’s quite annoying (basically, what #36022 was about). I have to blackbox the scripts manually using the right click menu whenever I need to debug async code. |
Node.js' internal scripts are not distinguished from user scripts. So when debugging a user script, the inspector is polluted with these internal scripts.
This could be solved either by blackboxing internal scripts by default, or setting a different script kind in V8 internally for these scripts. The latter probably requires an API change in V8.
@ak239 @ofrobots
The text was updated successfully, but these errors were encountered: