Should this be during eval instead of instantiate?

I did not dig deep, only did manual test. What I saw was that breaking during eval will actually break later then desired - e.g. for this example (console.log) it will actually break in the getter for the console. Variable declarations will also be skipped. Breaking during initialize worked as expected.

Nice to see this.

@eugeneo it may be worth trying some alternative wrapper functions as instantiate does sound fairly arbitrary as it does no module execution. Would job.run() at https://github.com/nodejs/node/pull/17360/files#diff-e6f2798544dae1f6ec863621e745489aR141 work perhaps?

The other thing that seems non-ideal is having that extra propery from within the ModuleJob class, it would be nice to be able to attach something from outside that way like:

initWrapper(moduleJob.run, moduleJob)

Let me know if any of these adjustments might work.

The main problem is that the breakpoint set from the C++ code will be hit when next JS instruction is executed. This means that we need to call this immediately before the user code is invoked. Otherwise breakpoint will be hit in the Node.js JS code and confuse/scare the user :)

E.g., replacing process.binding('inspector').callAndPauseOnStart(this.module.instantiate, this.module) with process.binding('inspector').callAndPauseOnStart(() => this.module.instantiate(), null) results in a break in the arrow function, revealing gritty implementation details.

From V8 and inspector pov, there is no difference between framework code (e.j. ModuleJob#run) and user code. But DevTools really want to break in the user code.

@eugeneo Is there a way to blackbox the inspector scripts by default?

Is there a way to blackbox the inspector scripts by default?

Chrome does not use blackboxing in scenarios like this so it had been designed to be controlled by the front-end. Currently blackboxing is per-session and is only available through the inspector protocol. E.g. Node would have to intercept Debugger.enable call and insert a bunch of new messages into the message stream. It is doable, but it might be risky and prone to breakages and may have sideffects.

@eugeneo thanks for the clarification, I guess I'm just completely not understanding how instantiate could be working here then, because instantiate does not do any execution, apart from initializing bindings and function declarations.

One of the tests I did was this script:

const a = 1;
const b = a + 1;

export default function() {
	console.log(10);
}

Breaking on initialization correctly steps over initialization of the a and b, allowing to see values. It also allows setting a breakpoint in the default export function.

@eugeneo I think that might be because you were breaking on the top level instead of the deepest leaf. Since ESM evaluates in post order traversal, the deepest leaf evaluates first.

This break is supposed to break once before any code was executed. There should be no modules loaded but the main one.

@eugeneo compiled locally and played around, I think this timing is the most consistent to how CJS --inspect-brk works. LGTM

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

CI seems to be unhappy. See https://ci.nodejs.org/job/node-test-binary-windows/13247/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=0/console.

Script URL format is different on different configurations. I will make the test more resilient, but it looks like there's an issue in the Node core.

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

@eugeneo this looks good, thanks for digging into this. I'm about to check out this branch and test whether the inspector stops in time for detailed coverage to be enabled in the inspector.

Windows CI is still unhappy. https://ci.nodejs.org/job/node-test-binary-windows/13277/

Just wondering what stalled here? Would be great to see this moving.

Btw @chrisdickinson has an interesting variation on this approach which may be a little bit more sensible, see 1d771f5#diff-46b46cfa0c77bff7ab6ed3fcb584e728R102.

It's probably worth someone closing related issues, it's not easy to navigate through the dupes of this one

@guybedford, @chrisdickinson's changes look good to me if you can file a separate PR, with the caveat that _breakFirstLine should be deleted right after the if clause.

@TimothyGu I tested out the approach and it seems to have the same problem as mentioned before of not exactly getting exactly the first line but instead getting loader internals. So this still remains the best approach it seems.

Landed through reworked PR in e7ff00d.

Awesome. Does anyone know when the next release will occur? I'm drooling over this one