GeezerVison

This was required because there seems to be some dependency cycle between esm_loader and the modules used during the resolution, causing destructuring to fail.

Ah, I think that might be because there are 2 instances of ESMLoader:

1. used "internally" during startup (ex to load loaders etc)
2. used for everything else after

See lines 71–86 below

Perhaps make the implementation a class so this is just instantiation?

One benefit of that would be treating the realpathcache as an instance property so that would give it a proper lifetime.

That does mean we have to think a bit more carefully about the lifetimes of the hook though too - this function is only called once though currently right?

I just looked at that, I think an object should be preferred over a class: first because we don't really use class features (in particular we can't use this since it'd break destructuring), but more importantly because it'd prevent the default fs functions from being observable via ObjectKeys. Given that it's currently the source of truth for the loader code to know what are the supported fs functions, using a class would require further special casing which I think makes the object form preferable.

I see pros and cons to both, but re-creating native functionality sounds slightly more on the con-side.

You could create an instance (the default instance / singleton) and get the keys from it:

class FSLoaderUtils { /* … */ }

const defaultFSLoaderUtils = new FSLoaderUtils();
const defaultFSLoaderUtilNames = ObjectKeys(defaultFSLoaderUtils);

function defaultGetFSLoaderUtils() {
  return {
    defaultFSLoaderUtils,
    defaultFSLoaderUtilNames,
  };
}

Is the plan to only use this in the esm/loader.js and not in the CJS loader? That seems like it might limit the applicability of these hooks, especially when CJS interfaces with eg exports resolutions.

At the moment the other hooks are only used in the esm code path so I didn't want to change that since it's not required strictly speaking. A followup PR however will be to consider getFileSystem for cjs.

Is there a reason we can't just have readJson as a well-defined abstraction on top of readFile? Package virtualization can still work by returning serialized JSON - which is likely needed anyway with threading workflows.

I didn't make perf analysis on internalReadJson, but I'd intuitively agree that a JS abstraction over readFile (that would just apply a regex to get the containsKey value) would fast enough without requiring a dedicated native function. I can experiment with that. Or do you have something else in mind?

In theory realpath can be implemented on top just an lstat and stat implementation, although that would be a slightly more complex refactoring of the realpath algorithm. It might well simplify the model though in just having three base-level FS primitive hooks to virtualize.

I'm a little worried about that - it'd likely be much slower, and the semantic loss could bring unforeseen issues. I'd not feel confident dropping it in this PR 🤔

Can we make sure to throw though if a specific loader implements an async hook but not a sync hook for the same method?

I think fsUtils might be a better name for this. Otherwise it sounds like internal vs external/public versions, but I think the important distinction is that these are utilities.

nit: this whole thing is the implementation

But I think this might be better as a class (like Guy suggests)

Can/should this be used to read json files that are not a package.json? If not, I think the name should be more explicit:

These can't/don't change, and they're also used in ESMLoader, so I think this should be moved outside defaultGetFileSystem() and be exported to avoid needlessly re-compiling this list.

Why shallow-clone this list?

It isn't clear which guarantees are made as to whether addCustomLoaders can/may ever be called from userland code, so I prefer to make the code handle that in a consistent way.

I may be wrong, but I believe it is not possible to access esmLoader directly from user-land without something very naughty like --expose-internals. But also, esmLoader#fileSystemBuilders is private, so it's inaccessible anyway.

Why make this a function that returns the current FS utils rather than just passing the current utils? (Ex this doesn't protect against mutating currentFileSystem)

Mostly to match the signature of other hooks, which provide the next hook function rather than their results (granted, they accept inputs whereas this one doesn't, so it could be possible, but by consistency I feel more comfortable keeping the same pattern).

eventually this should do a copy operation rather than direct reference to avoid people purposefully doing mutation becoming breaking change worries.

I woulda chosen k for "key" 🙂

It's not a key, though, but the index of a key. I looked at the existing codebase to pick the iterator name and saw many references to i,j

Merely a side comment—I wasn't suggesting a change. Sorry for the confusion. I use k as an index in a set of keys

for (const key of keys) // key = 'foo'

vs

for (let k = 0; k < keys.length; k++) // key = keys[k] = 'foo'

Ah, I think that might be because there are 2 instances of ESMLoader:

1. used "internally" during startup (ex to load loaders etc)
2. used for everything else after

See lines 71–86 below

it would be preferable to have this passed in as a param instead of grabbing off of the application context esmLoader (there can be multiple Loaders).

In which case does that happen? It could be a blocker for the node:resolution accessors I mentioned earlier, as it relies on loader utils being singletons.

If that's the case of the "loader loader" that @JakobJingleheimer mentions there, since from what I understand they aren't used at the same time, perhaps the global access would be fine?

I THINK their sequence is fully serial: the node-land esmLoaderNodeland finishes its work and disappears into the void, and then esmLoaderUserland steps in and starts its work.

Aside from Bradley's concern of bloating params, I think it would be better for this to be passed (and also makes it easier to test). One of our Loader team's side/wish-list projects is to make ESMLoader pure, and this goes against that. Not necessarily a show-stopper, but something to mention: If we do decide to make it global and preclude fully pure, we should do it consciously.

nit: ASCII order

We need to use destructuring here I think, otherwise monkey-patching of fs module would impact this module.

I think we should not make our user expect a promise here, if/when we end up getting rid of the sync methods in this hook, loader hook authors should expect the previous hook may have passed a synchronous function:

and if we want it to return a promise, we still need to avoid the spread operator on arrays (which relies on globally mutable Array.prototype[Symbol.iterator]):

Imo it'd be best to attempt to keep a consistent API - otherwise loader authors would be effectively forced to do:

const content = await Promise.resolve(fs.readFile(path));

The extra Promise.resolve wrapper wouldn't look very idiomatic.

Another solution is perhaps to just drop this "automatically bind sync functions into async slots", since it comes with its own drawbacks (it can be seen as a footgun, since it makes all async filesystem operations silently turn sync).

I'm not that attached to it, so dropping it would be fine by me.

otherwise loader authors would be effectively forced to do:

I don't think that this is correct, you can await non-Promise objects:

console.log({}); // {}
console.log(await {}); // {}
console.log(await Promise.resolve({})); // {}

I don't feel strongly about this, but imho it wouldn't be too bad as an API.

Yes: being able to await a non-promise was explicitly for this scenario (can find the citation if needed).

The conversion path -> URL -> path here is unfortunate :/

Indeed - I figured consistency would be important, but we could specify that the hook paths are either URLs or regular strings. I'm however a bit worried it'd just push to userland the responsibility of calling pathToFileURL.

There're A LOT of path → URL → path in ESMLoader/hooks.

I was thinking to propose anywhere a url currently is (within ESMLoader/hooks), it be replaced with a URL instance.