GeezerVison

I ordinarily try to avoid touching lines that don't otherwise have changes, but I had to tidy up code that was otherwise untouched because it was very difficult to read & follow (it looked like it had been through an uglifier—perhaps some kind of lint autofixer?).

@GeoffreyBooth I have a few questions:

There're very few code comments (at least in the ESM section), and it's very difficult to figure out what anything is doing / supposed to do. I haven't come across any docs that seem to cover this aside from "try to avoid unnecessary breaking changes".

I know this particular piece is intended to merely consolidate some methods, but as far as I've managed to glean, a lot of the ESM code needs ground-up change (ex how the Loader class is designed and used is…very strange). How much change there might you/y'all be open to?

Happy to jump on a call to discuss. I'm EST (Mondays and Fridays after ~17:30 would be good for me).

I know this particular piece is intended to merely consolidate some methods, but as far as I’ve managed to glean, a lot of the ESM code needs ground-up change (ex how the Loader class is designed and used is…very strange). How much change there might you/y’all be open to?

@guybedford is the expert on this part of the codebase, I think. Perhaps he can answer that. I would be happy to join a call but my schedule is difficult and I don’t know that part of the codebase too well either (just the hooks, for the most part).

I assume you’ve reviewed #35524? I was thinking that the first PR would just be completing what was started on that branch.

Yes, I reviewed it to help figure out what the code is doing. I'm still not 100% (probably closer to 70%). I'd like to run an approach by someone more familiar.

@JakobJingleheimer the code is split into explicit phases (resolve, getformat, getsource, transform). In terms of the system model of the loader, I'm not sure this is clearer. Additionally, you should run a lint pass on this code (make lint-js) locally, as it looks like it has a few issues.

This is also going to need a review for primordials. For example, array.push(v) should be ArrayPrototypePush(array, v).

Actually I'm now realizing this is changing the public loader API. I will remove myself from the discussion.

@devsnek i gathered about the phases ;)

It's a WIP commit; I didn't bother with the non-IDE-integrating linter yet.

On a related note, why 80 columns? Are we devving on iPads?

On a related note, why 80 columns? Are we devving on iPads?

The Node codebase is over ten years old, so some things you just have to live with at this point 😄 My biggest pet peeve is that its test suite is written in Python.

@devsnek is also a good resource for this part of the codebase, he knows it better than I do.

(Huge) issue for another time: would it be feasible and desirable to port the test suite to something JavaScript-based like Mocha? I’m pretty savvy at unit tests (but I dunno if I’m up for a Sisyphean labour on my own).

The tests themselves are mostly in JavaScript. The runner is Python. Remember, the test runner for Node was written before any Node-based test runner existed 😉 You won’t need to know any Python to write tests.

I wouldn’t spend time on things like trying to change Node’s linting rules. The project is very mature at this point and seemingly little things like that are big efforts now because of the number of open branches and collaborators and release lines that things need to be backported to, etc. Also changes like that would need sign-off by a huge number of people, and it’s just not worth the effort if it’s not blocking you from the immediate task you’re trying to accomplish.

Actually I’m now realizing this is changing the public loader API. I will remove myself from the discussion.

@devsnek The goal is to change both the public and private loader. See #35524. The goal is to complete that PR.

You’re right about primordials, but we can fix all those at the end once we have a working PR.

@devsnek Thanks for the tip about array.push → primordial!

Where do primordials come from? They appear to be magically available (no import/require). This morning I tried to find them via a file search on the repo, but didn't find where they're defined.

Okay, so since @devsnek and @guybedford aren't available for that call to discuss, I'll try to summarise here:

esm_loader.js instantiates ESMLoader¹ twice: first to use esmLoader.import(), then immediately instantiate a new instance (overwriting the previous) to pass the output from esmLoader1.import to esmLoader2.hooks (abbreviated / pseudo code):

hooks = (new ESMLoader()).import(userLoaderSpecifier)
(new ESMLoader()).hooks(hooks)
// esmLoader2.runGlobalPreloader()

ESMLoader.import uses:

• ESMLoader.resolve()
• ESMLoader.getFormat()
• ESMLoader.moduleMap

For esmLoader1, resolve and getFormat are node's built-ins.

I think instead:

• rename the Loader class to ESMLoader
• ESMLoader's constructor should accept/receive user loader specifier(s)
  • update esm_loader to instantiate ESMLoader only once
• ESMLoader's constructor should handle importing & caching the user loaders
• ESMLoader's constructor should automatically call runGlobalPreloadCode() at the very end of its execution
• update ESMLoader's hook properties (now globalPreloaders, resolvers, and loaders) to lists, pre-populated with node's default hook handlers

¹ the class itself is actually named Loader but imported as ESMLoader, so for clarity I'm referring to it as ESMLoader everywhere

I'm not too familiar with that section of the codebase, but in general this approach seems fine to me.

@JakobJingleheimer happy to answer questions and review, two things would help to do that:

1. Seeing the docs diff is the most useful thing to tell what part of the public API is actually changing here.
2. If you have specific refactoring questions, just listing them in a question that won't get lost in the thread would help to not miss answering anything.

update esm_loader to instantiate ESMLoader only once

We have an outer and inner loader to ensure that the hooks do not affect the loader modules themselves.

@guybedford sure! I'll update the docs over lunch today. Would you be available to chat about them this evening? (If so, mind DMing me on Twitter or emailing me?)

My questions are mostly around internal approach (I think the changes to the public API are pretty simple and agreed).

@DerekNonGeneric You should chat with @JakobJingleheimer here about this PR, I think this could be a good effort to collaborate on.

Yes please! I didn't expect this to drag on so long 😓

@guybedford did you see my previous message here? (I didn't receive any follow-up from you—did I miss it somewhere?)

When I try to build, I get the following error, which I don't know how to properly fix. I presume it's because I deleted the obsolete transform_source.js (which was pretty much empty anyway):

make[1]: *** No rule to make target `../lib/internal/modules/esm/transform_source.js', needed by `[…]/node/out/Release/obj/gen/node_javascript.cc'.  Stop.

I see in #37468 that file was replaced with a comment (presumably to circumvent this error). I can do that, but I think that's not the ideal thing to do.

@JakobJingleheimer feel free to DM me further to arrange a discusson, although written questions would always be preferable for context.

I'm trying to get the tests updated so I can verify the updates, but I don't recognise what it uses—seems to be something homebrewed?

I tried compiling, but I think it got stuck in a loop (make ran for like 20–30 minutes, and the output seemed to repeat after about 5). Possibly related: I have an M1 mac (and I know that's experimental).

I have tomorrow afternoon I can work on this. It would be a lot more productive not having to discover the context around the ESM subsystem (and test framework).

I didn't hear back from @guybedford, and I noticed @DerekNonGeneric has a note in his profile that he's taking a break from Node work.

@DerekNonGeneric has a note in his profile that he's taking a break from Node work.

I think he's back now.

Yeah, thanks for reaching out. I was planning to get around to doing this a bit later next month. We can continue work on your branch, but I am not 100% on the naming you have here. We can bikeshed in the #node-dev Freenode IRC channel if you're up for it (ping me on there tomorrow if you can).

Chatted with Derek, and he wants to revisit the consolidation discussion (to potentially reduce the hooks down to just 1). He's planning to return from his break in late April. Conceptually, I'm not sure if the One Hook to Rule Them All is logically possible (aside from that, I am a fan of simplicity).

I didn't get any answers to my questions so far, so I'm still groping around in the dark.

And now with the potential discussion revisit, I'm not sure what the status of this PR is and whether I should still try to figure out the unit tests.

@JakobJingleheimer, please keep this as a draft for now. I will address your questions momentarily too.