feat: add initial internal npm client and dependency resolver #15446
Conversation
| @@ -841,10 +842,11 @@ dependencies = [ | |||
| "ring", | |||
| "rustyline", | |||
| "rustyline-derive", | |||
| "semver-parser 0.10.2", | |||
| "semver 1.0.13", | |||
There was a problem hiding this comment.
Try to force a single version
There was a problem hiding this comment.
It seems like some crates are using a very old version.
| if package_folder.exists() | ||
| // if this file exists, then the package didn't successfully extract | ||
| // the first time, or another process is currently extracting the zip file | ||
| && !package_folder.join(NPM_PACKAGE_SYNC_LOCK_FILENAME).exists() |
There was a problem hiding this comment.
What will happen if a process locks and then errors out? Won't this lock file block all subsequent tries?
There was a problem hiding this comment.
No, right now it doesn't block any processes. There's a larger comment explaining it here and how it can be improved in the future for when there are multiple processes trying to extract the same package at the exact same time: https://github.com/denoland/deno/pull/15446/files#diff-c38e5f78351feb4850428062e3a3cc3796ca8ede71130bc2dd3b4760c9f2aa80R39-R46
| None => bail!( | ||
| concat!( | ||
| "Could not find package '{}' matching {}{}. ", | ||
| "Try retreiving the latest npm package information by running with --reload", |
There was a problem hiding this comment.
We should consider making --reload work just for npm packages like --reload=npm:
There was a problem hiding this comment.
Oh, yeah definitely. I wrote this down as a todo.
|
Feel free to ignore/dismiss/hide this suggestion if it's off topic. I don't use deno personally outside of some play projects, so I don't have a huge stake in this really, but I think this direction is cool, and have a bit of experience in this space. One thing occurs to me, though, you mentioned that the dependency resolver isn't complete, doesn't support peer dep resolution, etc. Have you considered the approach of porting @npmcli/arborist to rust? It'd be a relatively straightforward way to ensure that you are working with the same approach that npm uses, which for better or worse, has had a ton of play-testing, reflecting well over a decade of focused time and energy. So, like, even the less "elegant" ways it approaches some things got to be that way by modeling how users actually expect it to work (which is, quite often, completely irrational and inelegant! But humans gonna human.) Plus, it seems like there's a super good chance that a rust implementation of Arborist would be significantly faster and more memory efficient, and perhaps even be a way to speed up npm (that is, if it replaced the JS version entirely or in part). And even taking it on and failing would be a really great way to get fresh eyes on a lot of what it does, some of which quite frankly has probably never really gotten a thorough review. Probably a lot more work, but the result could be amazing for the whole JS community, and it'd be a very impressive project. If you do decide dig into it and have questions (or just complaints lol), I'd be happy to provide answers if I can. |
|
Hey @isaacs, thanks for the suggestion! We're planning on improving the approach to align more with npm's resolution in the future, but for this experiment we just want something quick to try it out. What we're building overall, doesn't exactly align with how npm works (more details in the coming weeks) and so I don't believe a straight port of arborist would work in this case, though it would be beneficial to look at the code to align the resolution more. With our upcomming approach, there will be packages that won't work, so we want to get an idea of just how incompatible it is (that said, I would expect it to be a lot more compatible than Deno currently is). If we find it doesn't work then we can pivot. |
|
Ah, I should have probably specified, I'm only suggesting the abstract tree building/resolution aspects of arborist might benefit from being ported for deno's use. The reification, packing, etc is probably mostly inapplicable. |
|
@isaacs yeah, definitely! We’d like to align the resolution as much as possible. We’re just going to get the high level pieces in place first, evaluate the approach, then start diving into these details. Probably won’t be for a few weeks though. |
No rush. Given my pace of OSS engagement lately, "a few weeks" may as well be "right now" 😅 |
|
If you have any questions about how npm does any of the other stuff, though, or just run into sticky puzzling situations, I'm happy to help if I can. |
|
Thanks! We'll definitely keep that in mind. |
Adds an initial very basic npm client and dependency resolver. It is missing a lot of things (ex. peer dependency support) and differs from npm's cli resolution—we're going to start with this implementation then work on improving it and building it up.
There are no tests for this yet because we're going to test this in integration once the other parts have landed.