update CoffeeScript loader for incoming hook consolidation

[JakobJingleheimer]

No description provided.

[JakobJingleheimer]

@GeoffreyBooth I think the trick of appending .js coffeescript-loader was using previously is no-longer possible with getFormat() and getSource() consolidated because the same url is used for both internally (whereas previously, the appended url was supplied to only getFormat, so getSource still worked). load() fails when it subsequently reaches the getSource step because the fake .coffee.js truly does not exist.

[GeoffreyBooth]

Why are these changes necessary?

[JakobJingleheimer]

Yes, very 😉

[GeoffreyBooth]

I don’t understand; are you saying that these changes were accidental?

[JakobJingleheimer]

Oh, sorry you asked "why". This is a bit of an edge-case, but when the cwd is not the node-loaders project root, this fails.

For instance, in my local:

~/Workspace/
  …/
    nodejs/
    node-loaders/

I cd into the nodejs dir, which has my pr-compiled node executable, and run:

~/Workspace/…/nodejs $> ./node \
--experimental-loader \
../node-loaders/coffeescript-loader/loader.js \
../node-loaders/coffeescript-loader/test.js

Without that extra bit of code, the files are not found.

[GeoffreyBooth]

Okay, that’s fine then. I think the most likely real-world case would be where the loader is in node_modules for the current project. Like say I add loader.js to CoffeeScript, then you’d run something like:

node --experimental-loader ./node_modules/coffeescript/loader.js app.coffee

So whatever we put here should work for this case as well.

[JakobJingleheimer]

Mm, I think the rule for this would be:

• refs to things internal to the loader module would need to be relative to the loader module (eg what you see above with import.meta.url)
• refs to things external to the loader module would need to be relative to the caller (process.cwd()).

[STRd6]

This breaks the tests when running on Windows with GitBash.

[GeoffreyBooth]

Since I wrote that code, I’ve learned that the arguments here should be URLs, not paths. That might explain why they were broken in Windows.

[GeoffreyBooth]

So is this a TODO? Could we spell out more clearly what is supposed to happen, and mention that this is a bug/unimplemented feature?

[JakobJingleheimer]

I'm honestly not sure what is supposed to happen—I expect this to work now and before. But it doesn't work then or now. I'm afraid to open a can of worms investigating this, but if it's supposed to work, I (biting my tongue so hard) think it should maybe block nodejs/node#37468 (depending on how pervasive the usage of this is).

What currently happens is the require.extension works, the transformed source disappears into the CJS loader (I confirmed it does get passed it), and then an empty object comes out the other side.

[GeoffreyBooth]

There’s no need to block a PR because it doesn’t fix an existing bug, especially not for an experimental feature. I think all we need to do at this point is add a comment here explaining something like “known issue: blah blah” with the clearest explanation you can write, and we’ll come back to it later. I just don’t want to forget about this or have this code in here and someone like you comes along and thinks that it should be working, or that it did work in some older version of Node but got broken somehow more recently.

[JakobJingleheimer]

Okay, so I did some debugging, and I think we have some kind of red-herring here: the cjs deps from coffeescript files do not hit ESMLoader at all. I added logging to ESMLoader::getModuleJob() (which all imports handled by ESMLoader go through), and it does not log anything for files imported by ./fixtures/esm-and-commonjs-imports.coffee. The only commonjs import it handles is coffeescript itself.

To be doubly sure I wasn't missing anything, I added copious logging to module translators' commonjs strategy, and there is nothing whatsoever for files imported by ./fixtures/esm-and-commonjs-imports.coffee.

So I think we can confidently say whatever is happening is not ESMLoader's fault.

[GeoffreyBooth]

So I think we can confidently say whatever is happening is not ESMLoader’s fault.

That’s fine, but we still need some way for users to write a loader that affects both ESM and CommonJS files. Like what I was trying to do with the CoffeeScript loader defining resolve and load but also require.extensions. It doesn’t need to be streamlined, like we can have people use the old require.extensions still if we must or if that’s the easiest way (at least for now) but there should be some way to have a loader that works for a project that has mixed types. I’d imagine that this might require changes to the CommonJS loader, which means we should absolutely come back to it later as a separate PR, but it would be good to put a comment here saying that we’re aware of the issue. Something like “TODO: Known limitation: currently, sources returned with format: 'commonjs' are ignored. This will be addressed in a future update.”

To take another example, say you wanted to create a YAML loader. As a user of such a loader, I would expect that both import data from 'data.yaml' and const data = require('data.yaml') should work when I have the loader enabled. Ditto for CoffeeScript or TypeScript or other more sophisticated file types that can have their own imports and are more JavaScript-like.

[JakobJingleheimer]

From the debugging I did, I did not see any way to get it to work as things currently are, but it may have just needed more investigation and/or more knowledge of the CJSLoader.

Regarding (future) updates to the CJSLoader, that sounds likely needed, but I remember someone (Bradley?) saying 2–3 team meetings ago that something like this would require changes to the CJSLoader, but previous efforts failed in a fiery blaze because there was too much disagreement.

I would certainly like it to be fixed, but if that's true, maybe we shouldn't promise the issue will be addressed?

[GeoffreyBooth]

Regarding (future) updates to the CJSLoader, that sounds likely needed, but I remember someone (Bradley?) saying 2–3 team meetings ago that something like this would require changes to the CJSLoader, but previous efforts failed in a fiery blaze because there was too much disagreement.

I would certainly like it to be fixed, but if that’s true, maybe we shouldn’t promise the issue will be addressed?

I think a comment can make whatever promises it wants. No one will hold that comment responsible 😄

To the broader point, though, I think we should tap Bradley (and Guy, if he has the time) to get their wisdom about what the issue is with supporting format: 'commonjs' and what kind of refactoring would be required to get that to work. Because it not working is pretty plainly a bug, and bugs must be fixed, or we change the API to remove format: 'commonjs' as one of the options. It can’t just be left broken because of disagreements; that doesn’t fly, and I think I could get the TSC to back me up on that one if necessary (but I doubt that would be necessary).

Anyway I think let’s add a comment and move on for now and get this PR landed, and after that maybe we can create an issue on nodejs/loaders spelling out the problem as detailed as we can, and one of the others can hopefully tell us where to look in the code for what needs to be changed to fix it.

[JakobJingleheimer]

Sounds good to me :)

I added such a comment to the CoffeeScript loader test file (lines 30–35). Does that suffice, or were you thinking to add the note elsewhere more conspicuous (like the ESM docs)?

[JakobJingleheimer]

Ah, okay, so I got the download from Bradley. The gist of it is what we thought: this part of the two APIs are not integrated. It is possible for the user to facilitate the integration by returning format: "module" instead of its actual "commonjs", parsing the source, and manually hooking it up to the exports, which doesn't sound pleasant but also doesn't sound like the worst thing.

@bmeck does that accurately represent what would need to be done from a high level?

[GeoffreyBooth]

The gist of it is what we thought: this part of the two APIs are not integrated. It is possible for the user to facilitate the integration by returning format: "module" instead of its actual “commonjs”, parsing the source, and manually hooking it up to the exports, which doesn’t sound pleasant but also doesn’t sound like the worst thing.

I think wherever the docs currently discuss format we should add a note. I don’t think we should add an example of how to do what Bradley is describing, since it’s clearly not the intended API, but I think we should document that for now format: 'commonjs' doesn’t work as intended or expected.

[bmeck]

@GeoffreyBooth to be clear it works as expected by my view XD. CJS hooks don't cleanly integrate against stuff.