-
Notifications
You must be signed in to change notification settings - Fork 21
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
Integrating pacquet to pnpm's existing codebase #78
Comments
Chiming in with my two cents, even though I know nothing about I think it's important to get clarity on what the objective is here, specifically. The one brought up is
This is a decent starting point, but like, is the goal here correctness of This matters because I'd make different tradeoffs in each of these cases. for example:
These are both decent options, but like all FFI, has some amount of overhead at the boundary. And I know perf matters to this project, generally. This style approach tends to work best if you can minimize the number of times you call back and forth between the two. So like, "we do argument parsing in js and then invoke pacquet to do all the work," that kind of thing could work well. But once you start to call back and forth, it may be more trouble than it's worth. This would also require users to have the rust toolchain installed, which matters a lot more if the intent is to have this in upstream.
This has the same shape as the node addon with the additional issue that you want to be making network and filesystem calls, and therefore are in the realm of wasi. you could structure this so that users don't need a rust toolchain involved, which is a pro.
This strategy is decent, but has an even bigger "back and forth across the gap" overhead. One that's not mentioned would be something like what many CLIs do: |
Thank you for your question. I have updated the issue description. |
I think we need to decouple pnpm tests and make them language agnostic. Later we can have the same test of integration tests run on both of the CLIs. |
@anonrig I don't think it is either necessary or possible to go as far as "language agnostic", just make the e2e tests independent from |
We have a registry-mock for e2e tests which can be for sure used in the pacquet tests. We have some e2e tests that test the pnpm CLI directly. Those could be shared potentially. But I don't think they cover a lot of the functionality. Most of the tests are package level. Like in the In my opinion the best way would be to start with writing a custom tarball fetcher for pnpm. A tarball fetcher is relatively easy to implement. I have already worked on a PoC, see the related issue: pnpm/pnpm#6663. If we can create a custom fetcher that will make pnpm faster (it won't be easy as I worked on some big performance improvements recently), then this part of pacquet will be tested by the hundreds of tests in the |
Rewriting all pnpm functionalities from scratch in pacquet is a huge task, so I think we should integrate with pnpm. Let pacquet handle tasks sensitive to performance and correctness, and pnpm the rest. It is also important to test pacquet against pnpm's tests and real world use cases. This would hopefully allow us to eventually convert the codebase to Rust (if we want to) without creating too many regressions or changing too many existing quirks.
How will we going to approach this? Should it be a separate process that communicate via IPC? Should it be a child process? Should it be a Node.js addon? Should it be a WASM library? Or should pacquet spawn pnpm as a child process instead?
@anonrig @zkochan
The text was updated successfully, but these errors were encountered: