Fix commonjs-extension-resolution-loader #10
Conversation
GeoffreyBooth
force-pushed
the
commonjs-resolution-loader
branch
from
0bcc48f to
d17c0ea
Compare
| const { parentURL = baseURL } = context; | ||
|
|
||
| // `require.resolve` works with paths, not URLs, so convert to and from | ||
| if (specifier.startsWith('node:') || builtinModules.includes(specifier)) { |
There was a problem hiding this comment.
nit: I think there's module.isBuiltin now
| } | ||
| const resolution = await resolveAsync(specifier, { | ||
| basedir: dirname(parentPath), | ||
| extensions: ['.js', '.json', '.node'], |
There was a problem hiding this comment.
I wonder if loaders should be able to expose their own hooks, for example to accept new extensions ... without that, a TS loader would have to implement the same thing but with a different extension, which would require to perform the same filesystem checks multiple times 🤔
There was a problem hiding this comment.
So ambient loaders weren’t enough, now we need loaders for our loaders . . .
There was a problem hiding this comment.
accept new extensions
I think the way to achieve this is for loaders to optionally load config files. There’s nothing stopping me from adding some code to the top of loader.js to look for a possibly existing config file, and if found to import it, and read the extensions list from there.
One loader could communicate with the following ones by attaching something to globalThis, I think, since that should be shared within the loaders thread (?). So that’s another way to define/share configuration.
There was a problem hiding this comment.
This is the sort of problem that I'm referring to when I say that the "helper functions" need to compose like hooks.
There was a problem hiding this comment.
Yes, they'll share globalThis (as long as we keep ambient loaders and lay loaders in the same thread).
But context may be a better vehicle, and will is guaranteed to work regardless of threading.
There was a problem hiding this comment.
Yeah, the blocker right now for the helper functions is the API design. I took a stab at it in nodejs/loaders#94 but I feel like that pass leaves a lot to be desired. A similar example is in https://github.com/browserify/resolve#resolveid-opts-cb: see the functions it accepts in the options to override its own lower-level operations.
|
W00t! Thanks! |
This loader should never have been part of the original repo, since it didn’t work 😄
This PR simply gets this loader working per its own test. As a follow-up I’d like to add all the Node codebase tests for
--experimental-specifier-resolution=nodeto ensure that they all pass with this loader.