testdouble / quibble

Makes it easy to replace require'd dependencies.
93 stars 26 forks source link

(bug): `isLoaderLoaded()` is always true #77

Closed Nkmol closed 1 year ago

Nkmol commented 1 year ago

A very minor bug, but still wanted to document this :)

What happened I am trying to use quibble as a standalone pure ESM mock framework, even though this is mainly targeted at TestDoudble. While looking at usage in TD (https://github.com/testdouble/testdouble.js/blob/main/src/replace/index.js#L16), I see the check to see if the loader is actually activated. The function of isLoaderLoaded, however, always return true.

What I expect When I do not use the quibble loader (--loader/--experimental-loader quibble) I expect the function of isLoaderLoaded() to return false.

Playground https://stackblitz.com/edit/node-ej1hze?file=index.js&view=editor


It seems that quibble.mjs, and thus the global quibble object identifier, is always loaded when importing the package (with and without loaders).

Nkmol commented 1 year ago

This is, as far as I can see, fixed in the new "support-off-thread-loaders" branch. Where the global identifier is defined by one of the loader hooks: https://github.com/testdouble/quibble/blob/support-off-thread-loaders/lib/quibble.mjs#L129

I was able to confirm that the following minimal implementation fixed this:

export function globalPreload(context) {
  return `\
    global.__quibble = { stubModuleGeneration: 1 }
  `;
}
searls commented 1 year ago

Hey @giltayar could you remind me of the status of that branch?

giltayar commented 1 year ago

I'm waiting on a major change on loader architecture (which moves loaders to a worker thread). Once that lands in Node.js, I will land that branch.

But! Theoretically, I can spend an hour or so making sure that the PR works in the existing Node.js versions (it should) and just push the PR without checking that the fix works when the loader architecture change lands in Node.js.

I think that's the way to go. With your permissions @searls, I'll go ahead and do it (and thus also close this issue)

(Sorry about the delay. It's holiday season in Israel ☺️)

searls commented 1 year ago

Go for it!

Sent from my Apple Watch

giltayar commented 1 year ago

So... I tried creating a PR for that branch and it seems that it doesn't work on Node 14. I mentioned this to the relevant Node developers (https://github.com/nodejs/node/pull/44710#issuecomment-1323213166) but for now it seems I can't land that branch.

searls commented 1 year ago

@giltayar yeah that seems tricky. What do you think we should do in the meantime for quibble/tdjs?

giltayar commented 1 year ago

I can try and fix the above bug. Or maybe get a PR from @Nkmol? ☺️