Ping @guybedford? (I've seen that you made most recent changes in this file)

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

If a refactoring could be made to avoid that resolution entirely that would be much much nicer (although would likely require another function argument)

At this point, it seems we could specialize the handling of --require a bit since at that point the users cannot monkey-patch module yet and there are certain exceptions (like this one) that should be considered.

If a refactoring could be made to avoid that resolution entirely that would be much much nicer (although would likely require another function argument), but otherwise this approach makes sense to me.

I tried to avoid changing the API as much as possible to avoid potential disruption. It's a fairly small change that can't break anything that was working before, which I think is safer in this situation than a full refactoring. What do you think?

This entire code path is a little messy, ideally it could be more like the process.mainModule === cjsModuleInstance comparison to avoid the need to double resolve at all.

I think we can't do this - it would fail for this kind of setup (but who does that, arguably):

node --inspect-brk --require ./foo.js ./foo.js

In this situation you will want to break inside foo.js during the preload step, since it won't get executed as main entry point (because it's in the cache). This makes it impossible to wait for the preload list to be fully executed, or to wait for mainModule or similar to be present.

Frankly it seems a super fringe use case, but I guess that's the rational behind the way the code is written otherwise I'm not sure why the ahead-of-time resolution was needed in the first place 🤔

I think we can't do this - it would fail for this kind of setup (but who does that, arguably):

Wait, doesn't the current logic also fail for this case?

In the esm resolver we have an isMain flag which is true through the load operation for the module that corresponds to the main entry into Node.js only - so that's the sort of logic I mean as an alternative.

Wait, doesn't the current logic also fail for this case?

No, because the break is execute inside _compile, so even for preloaded module.

In the esm resolver we have an isMain flag which is true through the load operation for the module that corresponds to the main entry into Node.js only - so that's the sort of logic I mean as an alternative.

There's also a isMain in the cjs loader (in _load). Still, the case described above wouldn't work anymore since the isMain field is only set when executing the main entry point, so not preloaded scripts. The resolution that happens in _compile is from my understanding a way to detect whether the script being compiled will be the main script - before it actually is.

Ah so you suspect this code path was exactly to handle this edge case in the first place, where the main itself could be in the module cache. Although it does seem very much an edge case as opposed to one to be justifying the base-level semantics on, but I guess that is where things are indeed.

Ping?

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

I'm not sure if the failing tests are related to my diff (I'd assume they aren't, since they're passing most platforms) - can I get some help?

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

It doesn't look related to me, I've re-triggered the failed builds.

The test failure here seems to be in async hooks -

test.node-api/test_make_callback/test-async-hooks-gcable
crashed (-11)

will try again and see if it works this time.

Actually no, the Travis report is wrong - this is all green.

Landed in 1549c8e.