esm: prevent erroneous re-initialization of ESMLoader

[JakobJingleheimer]

Fixes #43753

cc @cspotcode @daeyeon @nodejs/loaders

[JakobJingleheimer]

@cspotcode is

https://github.com/cspotcode/repros/blob/90feff7c486131f6c6e414f4ba7cd64f69dcec6d/index.mjs#L13

important? I ran the test 10 times without it and always got the same result (so I removed it for now).

[cspotcode]

I don't think it's necessary. I added it so that the logging showed the results of runMain before the results of the subsequent import.meta.resolve, to reduce potential confusion.

[guybedford]

Can't we just make runMain an internal function rather than a public one?

[aduh95]

Can't we just make runMain an internal function rather than a public one?

IIRC it's been (an undocumented) part of the CJS API for quite a long time, I'm afraid removing it would cause all kind of issues in the ecosystem at this point.

[guybedford]

Are there any reasons anyone would call it from userland that would actually do anything though? It can only be engrained if there is something to engrain?

[JakobJingleheimer]

@guybedford it was reported by someone from user-land, so at least one. If it's been around a long time, I would guess there are others.

@cspotcode how did you discover this—what were you using it to do?

[cspotcode]

runMain is used by ts-node to run the user's entrypoint script, since the "real" entrypoint is actually ts-node's bin.js.

That code predates my involvement in the project, so I'd have to do some digging to find the original commits. git blame will show recent commits that have moved, formatted, or indented things, but the actual introduction of runMain goes back further than that.
https://github.com/TypeStrong/ts-node/blob/aa5ec36526bf817b09345449492d5b9da11c0b93/src/bin.ts#L568-L579

I think this is the relevant snippet showing how it's used. We manipulate execArgv and argv, then call runMain. I'm pretty sure runMain looks at argv[1] and does the right thing to launch the entrypoint, handling ESM/CJS, and maybe some other stuff. I'm guessing that we really want to be as standard as possible, matching vanilla node, and that's why we use runMain.

[GeoffreyBooth]

Are there any times when the ESM loader should be initialized a second time? It seems like that's impossible now after this change?

[guybedford]

When @bmeck was working on the early loaders implementation, the runMain function was introduced as a private function for the CJS main function to be able to kick of an ESM main. It was originally called in the main code from lib/internal/modules/cjs/loader.js as the bridge into ESM. Later on the main was refactored out into lib/internal/modules/run_main.js. This was always an implementation detail and a private API. At the time the module API was still semi-private and not officially even fully documented. As a private API we can remove runMain.

The main reason for this function is to avoid an async bootstrap in Node.js. Otherwise we could just always defer to import().

runMain should be entirely equivalent to running import() against a module, and that should be a more than adequate replacement for it in ts-node. I think ts-node mistakenly used this private API thinking there is more going on here than that.

I'd strongly suggest ts-node just use import() and we deprecate runMain instead of carrying legacy that does no good.

[guybedford]

Actually my apologies I'm wrong - the original ESM implementation did not add runMain it merely extended it - #14369.

Note though that the module API was semi-private at the time and there wasn't even at that time any intention of it having a userland use case.

[cspotcode]

Pretty sure runMain also makes sure that legacy require.main === module checks will work.

[guybedford]

runMain does not set require.main. It just calls Module._load with a third isMain argument and that is it. If the file is an ESM entry point it just does an import(). The only exception is if there was legacy monkey patching of runMain, but that is due to be deprecated (https://github.com/nodejs/node/blob/main/lib/internal/bootstrap/pre_execution.js#L503).

[guybedford]

If there was monkey patching in runMain, calling it again would run the function twice as it is the second invocation. By definition runMain should only be called once - there is only a single main of a Node.js initialization process.

[guybedford]

Setting the third isMain argument to Module._load does set process.mainModule though and also applies some other very minor resolution details (symlink flags / extension searching). If passing an exact filepath these shouldn't apply though.

It's just important that we don't keep reifying weird patterns and actively work to reduce complexity in core when unnecessary. Loaders are so great to work with and maintain, I'm just slightly protective over the code. The more stuff that's there just because it's there the worse it gets to maintain in future!

[JakobJingleheimer]

Are there any times when the ESM loader should be initialized a second time? It seems like that's impossible now after this change?

I tried to think of a reason for that, but none came to mind.

However, I wonder if this might be a way to add loaders dynamically or a way that isn't via CLI args.

---

EDIT: PS adding request-ci just in case we do decide to move ahead (so we don't have to wait 59 hours for it to complete). I'll wait to land til the above "should we just make runMain internal" is settled.

[guybedford]

I'm fine with landing this, but it would be great to deprecate runMain as it's not a public or documented API and there are easy alternatives.

[cspotcode]

Based on my informal testing, the alternatives -- _load and import -- don't actually fit the bill. If an issue is created about deprecating runMain then I can share there. Maybe a case where #43408 can create a proper hook for this?

[JakobJingleheimer]

Shall we move the discussion about how to handle runMain() to the related issue #43753?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 820b710

[guybedford]

@bmeck added it in the original ESM implementation. There is no point in having it exposed to user land since it is idempotent and called in core I believe at the time there were more private APIs on module and there wasn’t as strict process separating private from public. I would really prefer we fix the original mistake and make it fully private.

…

On Sun, Jul 10, 2022 at 15:10 Jacob Smith ***@***.***> wrote: @guybedford <https://github.com/guybedford> it was reported by someone from user-land, so at least one. If it's been around a long time, I would guess there are others. @cspotcode <https://github.com/cspotcode> how did you discover this—what were you using it to do? — Reply to this email directly, view it on GitHub <#43763 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSS3OH7KLUM5SPWTI5DVTNC35ANCNFSM53FIYFKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>