Okay! So! Here I am!

The very first thing I want to say is, this code was already really great. If you were a random beginner who asked me to look at their code, I would say "great job" and not offer any of this feedback unless they were looking to actively level up at Rust. And while we don't know each other well, I do know that you're an experienced systems programmer, and looking to level up your Rust.

So I went a little extra. Sorry not sorry.

Each of these commits tells a story, in order. I left detailed commit messages. Some code gets re-written multiple times, but each step should be instructive, so if you only look at the final diff, you won't learn everything. Please take what you want, and throw away what you don't. And I am happy to discuss anything about any of this in detail.

This looks like a fun project!

• [x] Should respect to ^ version pins while installing dependencies. For example, [email protected] and [email protected] should not be installed if both of them has the ^ version pin. (https://github.com/anonrig/pacquet/pull/10)
• [x] Should not re-fetch same package multiple times. (https://github.com/anonrig/pacquet/pull/9/commits/fc0b3f40aa91f1efc3ba535f5e672b0195bd797a)
• [x] Should update package.json and add package to dependencies.
• [x] Add missing flags. Full list is available at https://pnpm.io/cli/add

This PR addresses the issue of inconsistent path handling across different operating systems in the get_all_folders function. The current implementation accepts both Unix and Windows path separators, leading to unpredictable behavior. The proposed solution aims to enhance cross-platform compatibility and improve code clarity

I think env should without $ symbols.

$ just for us to use it with bash.For example

echo $HOME

In rust , we should use it with these code:

let home = env::var('HOME')

Work in progress. Opening to receive feedback.

I'm not really happy with the architecture of the whole project and how registry sits on top of everything. Any recommendations @steveklabnik?

Until now pacquet just ran any script/shell commands by just directly passing the entire command to Command::new. As per the std, Command::new takes only the file/program name, and any additional args need to be provided via the arg or args method.

Well now, i just touched it a little bit, to run sh -c and pass the actual command/script to it. This runs without any errors now, and sometimes scripts contain env vars too. For example

{
  "scripts": {
    "test": "KEY=VALUE node index.js"
  }
}

Now it is passed as sh -c "KEY=ENV node index.js. This seems to be how the peeps do it in pnpm. (in pn)

And also pass any sort of additional args provided in the run command to the script. For example,

{
  "scripts": {
    "test": "echo Hello"
  }
}

Current running pacquet test and pacquet test World both just prints "Hello", but pnpm passes any additional args provided to the script too. So now it prints "Hello World" when done pacquet test World

An initial pretty simplistic benchmark that uses verdaccio (to remove the overhead of http requests to npm registry).

Benchmark 1: pacquet
  Time (mean ± σ):     364.6 ms ±  21.1 ms    [User: 44.0 ms, System: 173.1 ms]
  Range (min … max):   337.3 ms … 406.7 ms    10 runs

Benchmark 2: pnpm
  Time (mean ± σ):     945.9 ms ±  43.4 ms    [User: 585.6 ms, System: 1491.7 ms]
  Range (min … max):   876.5 ms … 1003.7 ms    10 runs

Benchmark 3: yarn
  Time (mean ± σ):     610.5 ms ±   7.5 ms    [User: 1250.2 ms, System: 181.2 ms]
  Range (min … max):   601.8 ms … 624.2 ms    10 runs

Benchmark 4: bun
  Time (mean ± σ):      1.055 s ±  0.222 s    [User: 0.079 s, System: 0.306 s]
  Range (min … max):    0.733 s …  1.550 s    10 runs

Summary
  pacquet ran
    1.67 ± 0.10 times faster than yarn
    2.59 ± 0.19 times faster than pnpm
    2.89 ± 0.63 times faster than bun

Bumps tempfile from 3.7.1 to 3.8.0.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Bumps reflink-copy from 0.1.5 to 0.1.6.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Bumps thiserror from 1.0.44 to 1.0.47.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Bumps clap from 4.3.21 to 4.3.23.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Before this PR, pacquet install was installing packages by "generations": All direct dependencies are installed first, then dependencies of direct dependencies, then dependencies of dependencies of dependencies, and so on. This was not optimal, but it was done to avoid race condition where some packages would be fetched twice should they be invoked in the window of time where its duplicated has begun fetching but not yet saved to local disk.

This PR aims to change this by using an in-memory hashmap cache to keep track of all fetched packages, then re-enable full parallelization.

NOTE: This PR only affects pacquet install, pacquet add still uses the old algorithm.

NOTE: New tests with multiple packages for pacquet install is required, but I am too tired right now.

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

before

Benchmark 1: pacquet
  Time (mean ± σ):     799.0 ms ±  17.2 ms    [User: 141.8 ms, System: 818.4 ms]
  Range (min … max):   777.5 ms … 827.8 ms    10 runs

after

Benchmark 1: pacquet
  Time (mean ± σ):     656.5 ms ±  26.4 ms    [User: 152.1 ms, System: 811.8 ms]
  Range (min … max):   625.6 ms … 709.3 ms    10 runs

The story of users

1. yaml file is not friendly to git diff. Almost every time in git diff, each diff block will be confused by same content from different rows and different projects as shown in the images below. This kind of code diff is not easy to read.

2. In a larger repository, there are usually many people working together, but their projects may be irrelevant. The lockfile conflict causes them to have to merge manually.

the solution I think

Inspired by Go's lockfile called "Go.sum", new format of lockfile stores the meta data of each packages in only one line.

The format I designed is like this.

before

after

// new-format.lock
// Nodes
[email protected] sha512-xxxxxxxxxx=  noDev
[email protected] sha-xxxxxxxxxx= hasBin noDev
[email protected] sha512-xxxxxxx= engines: {node: '>=0.10.0'} noDev

// Edges
[email protected] -> [email protected]
[email protected] -> [email protected]

Then, if I add a new version of loose-envify,code diff would become like this.

// new-format.lock
[email protected] sha512-xxxxxxxxxx=  noDev
- [email protected] sha-xxxxxxxxxx= hasBin noDev
+ [email protected] sha-xxxxxxxxxx= hasBin noDev
[email protected] sha512-xxxxxxx= engines: {node: '>=0.10.0'} noDev

- [email protected] -> [email protected]
- [email protected] -> [email protected]
+ [email protected] -> [email protected]
+ [email protected] -> [email protected]

The reason why I open this issue is that I think the new package manager should have more opportunities to try experimental features

So in #42, one issue that comes up multiple times is cache misses.

Basically, Registrymanager::get_package is racy:

1. look up package in cache, if it's there, return it
2. if not, fetch the package
3. insert package into cache

This works but has the property of a "race condition." (but not a data race!!!) If we spin up two threads, and both are looking for the same package, and thread 1 goes "1, don't have it, go to step 2 and fetch" and then thread 2 starts and does the same, we've fetched the package twice. This manifests as a cache miss.

The alternative is to cause 1 to block in some fashion. We want it to hold some sort of lock, so that if someone else wants something we're working on, we ask them to wait until it's ready. But there's not just one way to do this!

The first way, and the way that's most obvious, is that you put a mutex around the whole thing. That way, if someone is downloading a new package, you wait until they're done. The issue there is that you'd end up blocking everyone when a package is being downloaded, which kind of defeats the purpose of downloading them in paralell, haha.

So my next thought is that we need to do is change the data modelling from "package exists or not" to "package doesn't exist, package is being downloaded, package is complete in the cache." This more properly represents what's going on in the real world. But it's not actually the best way forward. Why? Well, what happens when we have a cache hit? We want to simply return the package. If we know that some other task is already downloading the package, what we actually want is to return. We have no more work to do! Sure it's not ready yet, but the other task will make sure it is.

So, I think that's the task to fix this issue. I don't think it should be a super complex diff, but I'm not going to try and write a patch right this second.