-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Node package-lock v2 #246
base: main
Are you sure you want to change the base?
Node package-lock v2 #246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for separating this out. This makes it easier to integrate your valuable work.
else if nodes ? "package-lock.json" | ||
then let | ||
# Wish there was a way to get the version without reading a 2MB file | ||
lockJson = getLockJson path; | ||
lockVersion = lockJson.lockfileVersion or 0; | ||
in | ||
if lockVersion == 1 | ||
then ["package-lock"] | ||
else if lockVersion == 2 | ||
then ["package-lock-v2"] | ||
else ["package-json"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of the translators
attribute is to list all possible translators.
If there is a package-lock.json and a yarn.lock, then both translators should be listed.
Also I think the new translator should probably be re-named to package-lock-v3.
The v2
file format just contains definitions for v1
and v3
parsers. There isn't really a v2
parser.
See https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion
Therefore if lock file version is 2
, both package-lock
and package-lock-v3
translators should be listed.
dream2nix will then pick the first on of these by default, so if we want v3 to be the preferred logic, it should be the first item in the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to make the v3 parser call the v1 parser if the version is wrong. Then the discoverer doesn't need to parse the json twice and the package-json impure translator can still use old nodejs versions. I'll give that a shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better for UX and debugging purposes if we could show the user specifically which translator implementation was used.
Actually parsing the json twice can already be prevented by making use of the tree
object which is given to the discoverer. tree
is created by lib.prepareSourceTree
and presents a directory tree as an attribute set. The attribute set also contains the json parsed content of each file. Because both discoverer and translator use the same tree object, json files will only be parsed once.
Instead of fromJSON
the discoverer should just use (tree.getNodeFromPath "/rel/path/to/package.json").jsonContent
.
Is it really necessary to have the impure package json translator work with older nodejs versions? Couldn't we just fix the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good tips, thanks, I'll work with those!
Is it really necessary to have the impure package json translator work with older nodejs versions? Couldn't we just fix the version?
I don't want to force a download of a specific version, but maybe I can check if the version of the nodejs pkg is >= 16 and otherwise use v18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavHau So, the pure symlink builder needs to have all peer deps and all circular deps for each dep, and neither package-lock v1 nor yarn v1 have that information.
The sure only way around that is to build the node_modules by first copying all sources into one place, and then running builds for every package. This is inefficient, with easily over 1000 dependencies sometimes. It's also tricky, clashing dependencies have to be detected and moved down into the tree.
The current builder+translator combo doesn't quite do that and fails in some cases (documented elsewhere).
For simplicity, I'd like to remove support for npm v1 and yarn v1 lockfiles. Then the pure builder has all the correct information for building a pure tree of symlinks. The disadvantage is that some projects will first need an impure npm step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplicity
If you isolate your features into a new builder/translator pair, I don't see how removing existing builders/translators simplifies things for you. We only need an interface to allow the user to chose between builders translators (which already exists).
I'd like to remove support for npm v1 and yarn v1 lockfiles
Users are currently depending on these modules. Removing that causes a lot of friction as it is unlikely that the new builders/translators will be bug free from the beginning. It will take some time to stabilize the new approach, and for that period, the old modules will still be useful.
I'd suggest the following upgrade path:
- add new logic as new translator/builder including an example under
/examples
on how to use it. (we can merge this before being fully stable, as it is an optional feature) - generate an index of the most popular 5000 nodejs packages (similar to dream2nix-npm). I'm happy to do take that task once we merged your new transaltor/builder.
- build the 5000 packages and fix bugs.
- once stable, make the new translator/builder the default
I think the test error may be because I'm adding source info for the main package? The reason for that is to be able to pass metadata like My next optimization will be to split the build into
This way, if the node version changes, it only rebuilds packages with install scripts. |
I noticed that a failed jsonschema validation did not lead to an error in the example tests. This is fixed in 9331fb1. You can see the schema that the dream-lock is validated against in src/specifications/dream-lock-schema.json. Instead of adding information to the sources, you can use the
Sounds like a nice optimization. But I'd suggest that we first integrate the simplest possible version of the new build logic. Probably as a separate builder. Then test this against many packages. Once we have confidence in it, we can make it the default. That might be valuable even without further optimizations. |
- discovers peer dependencies correctly - adds metadata like os, dev and hasInstallScripts to sources
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/yarn-plugnplay-and-direnv-packaging/19759/26 |
Support for package-lock v2 and v3 in dream2nix would remove a major hurdle for me in adopting dream2nix. Is there anything that I can help with to get this merged? |
in | ||
if lockVersion == 1 | ||
then ["package-lock"] | ||
else if lockVersion == 2 || lockVersion == 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have spent a bit of time trying to understand why dream2nix main doesn'twork and seems like it is because it doesn't support lockVersion 2. I would have much preferred for dream2nix to just bail out with "unsupported lock file version". I would have earned quite some time and this PR is the opportunity to do so for unknown/unsupported lockVersions
I was wondering why dream2nix wouldnt work with my project (lockfile version 3) until I stumbled on this. I am trying this out but nix can't find the git revision. I've been trying to run nix flake update (2.16) but it couldn't find mach-nix, the following fixed this:
then I tried to use my fork https://github.com/teto/dream2nix/tree/node-pkg-v2 on my project, which failed with
@wmertens would you mind rebasing this please ? |
@teto I'm kinda burnt out on this, maybe I'll revisit this but right now I have too many other projects. |
Putting this in a separate PR from #195 to split up discussions.