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

Is pnpm supported? #84

Open
TheLudd opened this issue Dec 4, 2022 · 10 comments
Open

Is pnpm supported? #84

TheLudd opened this issue Dec 4, 2022 · 10 comments

Comments

@TheLudd
Copy link

TheLudd commented Dec 4, 2022

I have a project using replaceEsm that works fine using yarn. But when I switch to pnpm it says that quibble cannot find one of my modules.

I assume this is due to the different node_modules structure of pnpm.

Is this a known issue? I am happy to supply more info about pnpm if you are unfamiliar with that package manager.

@searls
Copy link
Member

searls commented Dec 5, 2022

Hello! I had not heard of pnpm until a week ago, and all I know about it is that it dedupes all installations across a filesystem, so I'm disappointed but not surprised at all that quibble doesn't work.

Do you experience this only with replaceEsm (quibble.esm()) or also with replace (quibble())?

@searls
Copy link
Member

searls commented Dec 5, 2022

(cc/ @giltayar )

@TheLudd
Copy link
Author

TheLudd commented Dec 5, 2022

Only with replaceEsm.
Side note: I wanted to turn a commonJS project to an ESM project. Had to get rid of proxyquire and found testdouble. Step one: do that replacement, (everything worked). Step two, switch to esm and at the same time from replace to replaceEsm and that when the problems began. /Side note

I have had the same problem in another project where my code was supposed to find what you wanted to require from your file. This was with require though but I think the principle is the same.

To find the exact path, you can use this lovely Stability 1 util: import.meta.resolve

Problem is, you need the url to the file where I am replacing from. So I probably would need to pass import.meta.url to replaceEsm. Could be an optional argument but even so, I am not in love with the resulting function signature.
It probably would work though and then one can perhaps work out fom there how to beautify.

Following my stack trace, the error was in lib/esm-import-functions.js and that seems to be a commonjs file which left me confused.

@searls
Copy link
Member

searls commented Dec 6, 2022

Thank you for doing this additional legwork. The replaceEsm feature has been a little more turbulent for @giltayar to maintain because the official loader API has been a moving target and isn't quite settled yet. I fear pnpm adds an additional layer of complexity that will probably require detecting it and introspecting to figure out the real paths of the files

@TheLudd
Copy link
Author

TheLudd commented Dec 6, 2022

NP. I can do even more legwork if someone could explain what the purpose of lib/esm-import-functions.js is.
When I tried to edit it in my node_modules folder it comlpained about me using es syntax in a cjs file...

@TheLudd
Copy link
Author

TheLudd commented Dec 7, 2022

Update: I think I have solved the problem and will upload it as a new module soon. Stay tuned.

@TheLudd
Copy link
Author

TheLudd commented Dec 11, 2022

Here it is: https://github.com/TheLudd/fibble

I made a new library heavily inspired by how quibble did the resolve/load thing for esm modules.
I actually tried to do this about a year ago or so but could not figure out how to actually load the test doubles and how to keep the state synced between the tests that register the replacement and the files that request the replaced version. So by understanding quibble I was able to actually make this work. Global state FTW!

The library I made only targets es modules. It is also much faster than quibble, there seemed to be a lot of recursive calling being done in the loaders.

Anyway, I managed to use nodes own algorithm to resolve files and a loader hack in order to not need to pass in the url from the file requesting. Check it out and come with feedback if you want to :)

Ps @searls. This is the second time I copy one of your libs to make a test utility :)

@searls
Copy link
Member

searls commented Dec 12, 2022

Thanks, @TheLudd. I'll ask @giltayar to weigh in because on this front I really don't know what I'm talking about

@giltayar
Copy link
Collaborator

@TheLudd just looked at your code, and what you're doing is using import.meta.resolve to figure out where the module is instead of the hack I did (of using the loader itself to return the information).

Unfortunately, import.meta.resolve is experimental and even in the latest Node.js is available only via a flag, so we can't really use it. I thought about using a nice library (which I've used in the past) that polyfills import.meta.resolve (https://www.npmjs.com/package/import-meta-resolve), but this won't work if there are multiple loaders and some of those loaders alter the resolving of modules.

So, unfortunately, I can't incorporate your code back into quibble. But! If you could reproduce the problem in an example git repository so that I can check it out, I will definitely look into it and maybe fix the bug so that pnpm will also be supported.

@TheLudd
Copy link
Author

TheLudd commented Dec 16, 2022

Hi.
I am aware of the experimental issue, but the entire loaders thing is experimental with a non stable API so I don't consider the resolve thing a significant step in a bad direction. These projects will require the maintainers to be up to date with the changes in the nodejs api anyway. I will check out the import-meta-resovle package though and see if that works.

I think I will continue to maintain fibble and you can draw inspiration from it if you want to. I like how I managed to not be forced to pass in your own file url when you replace something in order to find the absolute path of the module to replace.

But! If you could reproduce the problem in an example git repository so that I can check it out

Reproducing the problem is very hard. I did try to do it but I think it requires a specific set of circumstances that are not easy to reproduce but likely to happen in a larger project.

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

No branches or pull requests

3 participants