search

testdouble / quibble

Makes it easy to replace require'd dependencies.
94 stars 25 forks source link

fix #496: remove recursive call to "nextLoad" #76

Closed giltayar closed 2 years ago

giltayar commented 2 years ago

This fixes https://github.com/testdouble/testdouble.js/issues/496

giltayar commented 2 years ago

This also surfaced a bug in ts-node/esm, so I'll send them a PR.

searls commented 2 years ago

Is a test feasible? Is there anything to do before merge?

giltayar commented 2 years ago

@searls I know! I usually don't add stuff without a test. But it's just too complicated. The recursion there in the loader hooks is too mind-boggling for me to fully understand. I tried, but just couldn't figure out how to distill the problem into a test. Mostly because I don't know exactly what happened there.

What I do know, is that now quibble's loader code is according to the loader spec, which it wasn't beforehand, so it's a good and proper change.

(Sorry, should have written all this in the PR description)

searls commented 2 years ago

Hey @giltayar, I invited you as a maintainer on the npm package (you already have commit bit to the repo). Here's a release doc. Want to try cutting this as a release?

giltayar commented 2 years ago

Hey @searls. I got the invite, but then I had trouble logging in to NPM (would you believe that?!). Support took some time to figure it out, but by that time, the invite expired. Could you resend?

(and thank you)

searls commented 2 years ago

@giltayar cancelling the invite from the npm interface, of course, failed. It did let me re-send it, apparently? Let me know if that works—otherwise I will also need to contact support

giltayar commented 2 years ago

It worked!