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

RFC for parent package.json #165

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Christian24
Copy link

What / Why

Adds an RFC for parent package.json which allows multiple packages to inherit fields/properties from a parent package.json similar to Maven or Gradle.

This is my first RFC. It's certainly not perfect, however I do hope it helps to kickstart the conversation on resolving some of the issues I mention in the RFC.

Feel free to make edits or give constructive feedback.

Looking forward to get into a conversation with all of you,
Christian

References

  • n/a

@ruyadorno ruyadorno added the Agenda will be discussed at the Open RFC call label Jul 15, 2020
@isaacs
Copy link
Contributor

isaacs commented Jul 15, 2020

Some initial thoughts:

  • If we have to munge the package.json coming out of a tarball (and potentially fetch something remotely at that time) then that'd be pretty onerous.
  • Worse, if we have to make HTTP requests to read package.json files living in node_modules, also pretty bad.
  • So, if we do anything like this, I think we'd have to say that the package.json file that lives in a tarball is resolved against its base package.json at pack time.
  • Given that, if the base package manifest changes, now you have non-deterministic builds. (Ie, running npm pack or npm pubblish on the same folder multiple times isn't guaranteed to produce a consistent artifact.)
  • Given that, I don't think we can support a semver range, though a <base-package-name>@<specific-version> might be feasible (since that can never change, at least within a given well-behaved registry).
  • Even with that restriction, though packages can be unpublished. What happens if the base goes away?
  • We could radically simplify this by saying that the extends has to refer to a specific file, and refuse to pack/publish anything with an extended package.json. (Or, at least, resolve the extension and put the resolved package.json in the tarball instead of the actual file.) Then you might get non-deterministic builds if the base package changes, but that's a local file change (and obviously we can't pack consistent artifacts if the files you're packing change).

I'll look more into how Maven and Gradle do this, see if there's any inspiration we can draw there. I can certainly see the utility in projects using workspaces with a lot of repetition. (We have this ourselves in npm, where a bunch of our packages all have a lot of the same fields.)

@Christian24
Copy link
Author

First of all, thank you very much for getting back to this. My kinda initial reaction, I will try to see what I can do on the weekend, but I do not know much about the inner workings of npm, so mistakes will be made along the way :) Also, I I know my idea is probably impossible as is, because I mostly just thought about what would be helpful, not reasonable.

Some initial thoughts:

  • If we have to munge the package.json coming out of a tarball (and potentially fetch something remotely at that time) then that'd be pretty onerous.
  • Worse, if we have to make HTTP requests to read package.json files living in node_modules, also pretty bad.
  • So, if we do anything like this, I think we'd have to say that the package.json file that lives in a tarball is resolved against its base package.json at pack time.

Interesting idea. I will try to figure out how Maven handles this. I guess your objection is that this might spiral out of control right?

  • Given that, if the base package manifest changes, now you have non-deterministic builds. (Ie, running npm pack or npm pubblish on the same folder multiple times isn't guaranteed to produce a consistent artifact.)
  • Given that, I don't think we can support a semver range, though a <base-package-name>@<specific-version> might be feasible (since that can never change, at least within a given well-behaved registry).

Could this be circumvented when resolving the parent package.json at install? Since then npm would try to get a published version from cache or the registry...

  • Even with that restriction, though packages can be unpublished. What happens if the base goes away?

I see two options:

  • the once resolved tree is kept in the package-lock.json
  • you get a 404 (which I think is what happens if a dependency is unpublished). Honestly, I think this is not really an issue if it happens. I think packages and parent package will somewhat always be related, which is to say they belong to the same organization, scope, project or team. Yes, this can happen, but I am not certain it is likely. I do not see that people pick a parent that is not under their control, that might be unpublished. I for example wouldn't pick the Typescript project's package.json to use it as a parent. What would be the benefit? However, I could see people providing "starters" for Babel, webpack etc. but then authors would be aware that other packages depend on their work. With any dependency it could happen that it is unpublished.

Although, the proposal goes in a different direction it might make sense to think about creating a specific format for parents. This would prevent extending from unsuspecting packages. This would also allow to add specific metadata, which might make resolution easier.

  • We could radically simplify this by saying that the extends has to refer to a specific file, and refuse to pack/publish anything with an extended package.json. (Or, at least, resolve the extension and put the resolved package.json in the tarball instead of the actual file.) Then you might get non-deterministic builds if the base package changes, but that's a local file change (and obviously we can't pack consistent artifacts if the files you're packing change).

I am sorry, but I am not sure I understand what you mean here. Do you mean building a resolved package.json before publishing it to the registry?

  • I'll look more into how Maven and Gradle do this, see if there's any inspiration we can draw there. I can certainly see the utility in projects using workspaces with a lot of repetition. (We have this ourselves in npm, where a bunch of our packages all have a lot of the same fields.)

Thanks. I will try to update the RFC this weekend. It is encouraging you can see the utility too, so I was not entirely on the wrong track.

Thanks again and looking forward to shaping this out further.

@Christian24
Copy link
Author

@isaacs I updated the proposal and hopefully addressed some of your points.

@naugtur
Copy link

naugtur commented Jul 22, 2020

As per the meeting conversation, a partial solution you might use with features that exist now is installing packages as relative paths to packages installed top level: https://docs.npmjs.com/files/package.json#local-paths

@Christian24
Copy link
Author

I updated the RFC as discussed during the last meeting. It slimmed down tremendously, but also has a couple new facets.

@isaacs
Copy link
Contributor

isaacs commented Jul 27, 2020

If we resolve parent packages at install time, it gets us into very brittle territory.

I do not share your confidence that no one will be foolhardy enough to extends a package that they don't control. If we support it, even by accident, it will be depended upon. That is just the way of things in a big community of users :)

So, a package might be unpublished. That would be unfortunate, but could happen, and likely not much worse than a dependency going away. So, not a great user experience, but not particularly hazardous, as things go. It would mean that we'd have to start tracking extends as a sort of dependency semantic in the registry, just like we do for deps today, which prevent unpublishing in those cases and avoid the worst impacts. Non-zero work, but not rocket surgery.

I have two much larger concerns about this. The first is about resolving extends at install time is multiple levels of extensions, and performance issues resolving all of them. The second is that it means we lose reproducible builds.

So, I have package a that extends b, which is published to the registry. Then package b extends c, also published to the registry. c extends d, and so on, down to package z.

Let's say our project depends on a. I install on Monday. You install on Wednesday, using the same lockfile I created, at the same git commit that I did, so no changes whatsoever in our content. However, a new version of z was published on Tuesday, so you get different thing being installed. That's a big hazard.

In fact, since a new version of an extended package could be installed at any point during the install process, you might even have two duplicated instances of the same package at the same version that are actually different. Nailing down any kind of consistency guarantee wouldn't be impossible, but it'd be quite a lot of complex moving parts to get exactly right, both on the registry and in the CLI. I wouldn't want to sign up for that, personally, and it'd take much more work to implement.

If we instead say that extension only is allowed for the root project, and that the package.json that goes into the tarball and registry manifest is a point-in-time resolution of the extension, then that simplifies things greatly. You could still get the situation where a new version of b was published, changing the build state of project a above. But, anyone depending on the published version of a would be safe from any impact. Still can't guarantee reproducible builds for a given set of contents in projects that use extends, but we could say it's reproducible within a given set of contents and version of the extended parent package, which is a perfectly reasonable caveat to using this feature, imo.

@Christian24
Copy link
Author

@isaacs I agree. I had already updated my RFC last weekend, because of what was discussed on Wednesday. I don't know, did I miss something when updating the PR?

I do agree that the resolution should happen completely during pack. In my draft last week I had still the option of allowing the user to update on the fly if the parent has a new version. However, this would create a state with an updated parent that does not really have a version (for the child package) and all sorts of complications. And to be quite frankly, it does not have a real advantage over resolving during pack. So, I removed it completely from the current draft.

The draft now basically says that resolving happens during pack. This should help not having to update read-package-json-fast if I understood you correctly. Plus, it leaves us with a limited set of features we need to support locally (during package development or in a workspace). I guess these could all be implemented by touching read-package-json.

One of the interesting cases I guess would be if you extend package A in package B and A is also a devDependency of B. This could potentially happen if package A was typescript for example. I think this would turn out to be complicated, because if I run npm update A which A is updated? Honestly, I think we should just prevent this from happening. Is there a precedent for that? I just tried using the same package as a dependency and devDependency, but it seems it is only written in one section.

@Christian24
Copy link
Author

So, I tried my best to incorporate as much of the feedback from today's meeting as possible.

Here are the things I (hopefully) addressed:

  • How local commands such as ls or outdated work. I noticed npm does not really group or mark dependencies (unless you specially tell it to), so I added an argument --parent, similar to --dev or --prod. Initially, I wanted to avoid this, but I feel this is more in line with how things are now. So, to my mind that is better. I also added examples on how these would work in some more problematic scenarios like the update one I mentioned.

  • @isaacs I removed the restriction of not being able to inherit foo and also having it a dependency in the child. My concerns are addressed with the edits mentioned above.

  • I added a restriction about circular dependencies. Thanks, Wes. I did not add much as to what should happen if this is violated though, for now I would only fallback to what's currently there.

  • @darcyclarke I do not feel comfortable enough writing much about the package-lock.json (yet). I feel this should not be missed, but it also feels like it should go in the implementation part, which is still completely empty. However, I think it is useful for developers to know which version of the parent exactly was resolved, so I added it to npm list as an extra output. This kinda implies it will get added to package-lock.json.

I hope there's no feedback I have missed to incorporate. Otherwise, please let me know and I am terribly sorry.

BTW: I just installed npm 7, it works great so far and it made me already close an RFC I wrote this afternoon. 👍

@ljharb
Copy link
Contributor

ljharb commented Aug 19, 2020

(as discussed in the RFC call today)

Currently, package.json is static, which means that tools like https://github.com/browserify/resolve (which i maintain) are able to both synchronously and asynchronously make resolution decisions based on their contents. Even if npm were to provide a new API - both async and, scarily, sync - that provided the fully flattened package.json contents - it would be a significant burden on the JS ecosystem to have to adapt to this.

An alternative that was suggested was having package.json remain static, but having a different file that npm uses, when needed, to generate a static, flat package.json. This would effectively sidestep almost any tooling impact, since the only tools that would have to change are ones that validate or write to package.json (where they'd possibly want to validate the source manifest, and where they'd definitely want to write to the source manifest).

@Christian24
Copy link
Author

@ljharb I think I see what the problem is. Thanks for bringing that issue up. I will try to incorporate this into the RFC.

@Christian24
Copy link
Author

@ljharb I have updated the RFC. This should mitigate tooling impact. It would be cool if you could give it a read and give me some feedback.

One problem I see is what happens when a user edits package.json by hand. Those changes could potentially be overriden. Potentially, npm could warn if there are further changes in the package.json.

@ruyadorno ruyadorno removed the Agenda will be discussed at the Open RFC call label Sep 8, 2020
@settings settings bot removed the Proposal label Sep 21, 2021
@lukekarrys lukekarrys added the Agenda will be discussed at the Open RFC call label Jun 9, 2022
@wesleytodd
Copy link

@Christian24 do you mind if I pick this up? The npm team presented a roadmap for upcoming releases and this was not on it. I really think this one is great, and I would be happy to co-champion it with you or take it over completely depending on your availability and desires.

@wesleytodd
Copy link

Hey @Christian24, since I haven't heard back on this I am going to assume it is alright if I pick it up. I am not sure if I can re-use your existing PR (I will try), but I will for sure keep your commits if I have to open a new PR.

@Christian24
Copy link
Author

Christian24 commented Oct 11, 2022 via email

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

Successfully merging this pull request may close these issues.

None yet

9 participants