GeezerVison

Review requested:

• @nodejs/loaders

If one of the modules has a top level await you have to load the modules in order, but if not you could load them unordered.
So it is a fix which can have performance regressions?

So it is a fix which can have performance regressions?

I think it's more accurate to say this has performance implications.

has a huge impact on performance?

You say huge, how big are we talking about?

has a huge impact on performance?

You say huge, how big are we talking about?

That's a good question that deserves a benchmark 👍

I’m -1 on this. Is it really worth preserving the order when sequential awaiting has a huge impact on performance?

Yes, because the current implementation doesn’t achieve the desired effect. If the first --import registers hooks to handle TypeScript, the second --import of a TypeScript file should work. It doesn’t matter how much slower it becomes; the current behavior is broken.

If the user wants parallelized loading, they can put a bunch of import statements into a file and --import that.

has a huge impact on performance?

You say huge, how big are we talking about?

That's a good question that deserves a benchmark 👍

I don't think it's that good of a question, it was mostly rhetorical. The change has literally no impact on folks who are not using --import, and the impact of folks using lots of --import will depend a lot on what's inside the imported modules. I don't think we can come up with a benchmark showing regressions unless it's very artificial (i.e. no real-world use case would be impacted), but happy to be proven wrong.

--import is an experimental API, performance consideration is not a strong enough reason to block that PR from landing IMO (unless you're saying it will slow down users who are not using it, but I don't think it's the case).

I’m -1 on this. Is it really worth preserving the order when sequential awaiting has a huge impact on performance?

@GeoffreyBooth supplied one reason it has to be sequential. Another reason is the "loader registration use case" as outlined in #50427 (and which was how this bug was found), where if the user writes --import loader-a --import loader-b then they want to register the loaders to be registered in that specific order because loader chain order is important: some loaders have to be chained in just the right order.

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

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

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

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

Landed in a3e09b3