Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Resolve 3rd party esm without quibbledUrl #115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andreek
Copy link

@andreek andreek commented Sep 11, 2024

In Node.js >= v21.0.0 quibble adds 3rd party libs with ?__quibble=0 suffix as key to the quibbled modules. When Node.js is resolving the real module quibble does not find the mock, because this url does not include ?__quibble=0.

I think it's related to these API changes: #96

This PR fixes this problem by excluding urls with node_modules from being resolved with quibbledUrl.

Fixes: testdouble/testdouble.js#530

@quinnturner
Copy link

You may also want to consider other node resolution algorithms like PNPM and Yarn, both of which use symlinks that may result in not having a node_modules folder. For example, including .yarn or .pnpm.

@andreek
Copy link
Author

andreek commented Oct 16, 2024

You may also want to consider other node resolution algorithms like PNPM and Yarn, both of which use symlinks that may result in not having a node_modules folder. For example, including .yarn or .pnpm.

Thanks for the hint but I don't see a problem if I'm not missing something. I've used PNPM to develop this PR and every symbolic link includes /node_modules/ as suffix. I'm not that familiar with yarn, but I don't run into a problem with it either.

@marcoreni
Copy link

I can confirm that this PR makes quibble work with Node22. I'm using PNPM, so no troubles on that side.
@searls any chance of getting this merged and released?

@VadBary
Copy link

VadBary commented Nov 11, 2024

HI @andreek, this issue is a blocker to migrate to new NodeJS version. Is this possible to merge the fix?

@OlyaFreedom
Copy link

I'm also looking forward to this fix 😔 @searls could you please help with reviewing and merging this?

@OTod
Copy link

OTod commented Nov 14, 2024

Up-voting for this. The issue in test-double/quibble+node >v20 is a moderate blocker for us 😔 Kindly ask @searls to assist with incorporating the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suite test:esm fails under Node 21
6 participants