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

Add support subcommand #246

Closed
wants to merge 8 commits into from
Closed

Add support subcommand #246

wants to merge 8 commits into from

Conversation

kemitchell
Copy link
Contributor

@kemitchell kemitchell commented Sep 6, 2019

This PR supersedes #187. Many thanks especially to @ljharb and @wesleytodd there.

  • Pave a very clear path for adoption of a broader data schema from the Package Maintenance Working Group.
  • [ ] Consider supporting existing formats, like sponsors.yml from raw.github.com.
  • [ ] Play more aggressive defense on HTTP GET requests. Request body limits. Timeouts. Max redirects.

@ljharb
Copy link
Contributor

ljharb commented Sep 6, 2019

One thing that might mitigate some of the concerns about URLs being ephemeral, is if inside node_modules, the support JSON file was reified on install into an actual file (or, alternatively, if the data was cached on the filesystem locally such that it could be backed up and accessed offline).

@kemitchell
Copy link
Contributor Author

@ljharb, persistent caching had crossed my mind, too. Specifically, I’d thought about including fallback data in package.json. Clients would first try the URL for the latest. If that doesn’t work, they could fall back to the hard-coded data, with a warning. But I wonder why I’d ever want data that are clearly out of date.

As I mentioned on a WG issue, I’m still a little fuzzy on the actual use cases the WG is designing for. The use case I see here is a small minority of users running a subcommand a small minority of the time to generate reports on how to donate to, hire, or otherwise pay the people and orgs behind their deps.

@wesleytodd
Copy link
Contributor

One thing that might mitigate some of the concerns about URLs being ephemeral

In the call on Wednesday @isaacs mentioned having the registry host the file. If that can be implemented in a not unreasonable time frame we could just skip this whole conversation right?

If the data was in the pacakge.json it would stay there. If it was a url, it is slurped up into the registry at publish time (de-duped if it hasn't changed and left as is if the url is un-resolvable). Since it would be at publish time we would have access to previous versions of the data at publish time, it would be forever available, and could be inlined into the packument (or wherever is best for accessing w/o downloading the whole tarball).

Does this sound like a reasonable approach?

Pave a very clear path for adoption of a broader data schema from the Package Maintenance Working Group.

❤️

Consider supporting existing formats, like sponsors.yml from raw.github.com.

👎 If we add this complexity we make the tooling more difficult. I personally am a fan of saying specifically json.

@ljharb
Copy link
Contributor

ljharb commented Sep 6, 2019

also whatever formats are supported, please never yaml

@isaacs
Copy link
Contributor

isaacs commented Sep 6, 2019

Caching and locally copying is an implementation detail. It's unlikely that we'll put it in package.json specifically as of npm version 7, but it'd be cached somewhere for sure. My preference is that it be transparently cached in cacache, obeying the server's cache-control headers, like everything else we fetch.

I believe I'm in agreement with @kemitchell about this, but just for posterity: placing this information in the package.json file, or any other immutable place, is a terrible idea which will undermine the entire point of doing this. Support information is mutable data. As currently drafted, even target engine support is mutable, because "target":{"node":"maintained"} changes over time.

The ephemerality of urls is a feature in this case, not a bug. If the url goes away, so does any assurance of support, and a package is back to the default null state.

@wesleytodd
Copy link
Contributor

wesleytodd commented Sep 6, 2019

implementation detail

In this case I am not sure I know enough of the internals of npm to say much on this. I think the behavior tooling authors would want is a file in node_modules and a url which returns this file for remote packages without downloading and unpacking the tarball.

placing this information in the package.json file, or any other immutable place, is a terrible idea which will undermine the entire point of doing this.

Putting this information anywhere else today also has large drawbacks. A separate file in the package means unpacking the tarball. An external url means it can (accidentally) disappear. My assumption is that when you say "terrible idea" you mean that you see a future where package.json is the least optimal solution. Do you have a proposal which solves all three requirements?

  1. Performant and simple access
  2. Cannot disappear
  3. Is not in package.json

even target engine support is mutable

You are correct that that is a moving target, but the target is a an intent, and that is not changing. Saying "our goal is to support node LTS releases" can be an unchanging statement from maintainers even as the LTS tag moves.

This is the difference I was talking about between engines and support targets, although I didn't describe it well when put on the spot on the call. Engines is "what this code runs in", which is different than "this is what this code should run in". If either are not true, it is a bug. But one is about the machines and one is about the humans.

EDIT: "should run in" is not even accurate, it can run in many other targets, but still not be supported by the humans. So in this case engines and target can be different.

The ephemerality of urls is a feature in this case, not a bug. If the url goes away, so does any assurance of support, and a package is back to the default null state.

We had this out on the original proposal. IIRC I even made this point, and was convinced otherwise. A few points I remember off the top of my head:

  • One expired cert or domain name reg does not mean that a package in unsupported.
  • The support metadata is a "statement of intent", and having the statement around even if the ability to live up to it is gone is a good thing.
  • The addition of the "expires" fields was to ensure that the permanently accessible metadata still has a shelf life.
  • If it is not cached immutably somewhere we loose out on the ability to retrospect. I think in the long run we want that ability.

@kemitchell
Copy link
Contributor Author

kemitchell commented Sep 6, 2019

I’d hasten to add only that, from a certain point of view, the whole matter of how well-structured data about the people and orgs behind packages get to package consumers is implementation. In the end, consumers will get somedata somehow. It’s up to the WG and other folks writing schemas and semantics to make them meaningful and actionable from there!

I think all involved are going to have to rely on the WG especially to bring the institutional consumer perspective to that effort. On the producer side, despite all the implementations out there, it actually seems pretty clear what producers want these days: a way to show links to webpages and profile pages on various payment, fundraising, and market platforms to consumers. That’s the main reason I would personally—not as an npm person here—like to see producers’ means of sending their pleas temporarily decoupled from consumers’ means of receiving risk management information, if the latter will take longer.

Pending whatever the registry or npm’s profile or orgs systems end up doing, I think a simple URL pointer is a good, generic bridge. If folks end up storing WG data on the registry, they can always point to it by URL. Or perhaps registry-hosted data becomes the default at that point, to be sent down with every install, like audit data. Then only packages with URLs in package.json and no corresponding registry payload prompt a fetch.

@isaacs
Copy link
Contributor

isaacs commented Sep 7, 2019

In this case I am not sure I know enough of the internals of npm to say much on this. I think the behavior tooling authors would want is a file in node_modules and a url which returns this file for remote packages without downloading and unpacking the tarball.

I don't get the opposition to downloading and unpacking a tarball? Is that coming from you, or others in the package support WG? Because unpacking a tarball is not hard. I can show you how to do it in a few lines of code. But also I'm not sure what solution would require that, where it wouldn't also be in the manifest (which only requires an unpack for metadata in the case of git or file:.../package.tgz deps).

  1. Performant and simple access
  2. Cannot disappear
  3. Is not in package.json

1 and 3 are good, and relatively easy to accomplish. 2 is bad, is what I'm saying.

Just spitballing here, on a performant and simple-to-access system: if there's a url in package.json, then the registry bundles the support info in manifest and packument requests, and respects cache control headers when storing the upstream response.

This also has the advantage of not requiring it to be attached to a specific artifact. I could say "for $X/month, you can get support for all my packages, or this subset of them" and we could programmatically send that down to consumers, or allow them to filter, or see the current status in other channels.

One expired cert or domain name reg does not mean that a package in unsupported.

I'd argue it does. If the home of the data falls down, the data ought to fall with it.

If the izs.me domain name reg expires, then my blog goes away.

If we want to solve the problem of "mutable resilient data", there are other avenues to explore. But all of those other avenues would provide a URL to a resource. This is an orthogonal problem, unrelated to package support.

The support metadata is a "statement of intent", and having the statement around even if the ability to live up to it is gone is a good thing.

No, omg, no, please stop, that is definitely not a good thing. This is a bad thing. Inaccurate data that leads people to rely on me when they cannot, can only cause hurt feelings and damage my reputation. Trust me, this is very bad. It harms human beings in real ways. I am not trying to be overly dramatic here, but I have been harmed by it in the past, and have only survived as long as I have in open source by hardening my heart, which is too much to ask of anyone.

The addition of the "expires" fields was to ensure that the permanently accessible metadata still has a shelf life.

What's the default expires value, if not set?

What is the semantic meaning of an "expired" support intention?

Does that field mean that my income stops being advertised at some predetermined point in time unless I either continue publishing updates or agree (either foolishly or in bad faith) to provide support for eternity?

Even with a shelf life, an immutable representation of fundamentally mutable data leads inevitably to incorrect data. When dealing with human reputations, expectations, and livelihoods, it is harmful.

If it is not cached immutably somewhere we loose out on the ability to retrospect. I think in the long run we want that ability.

This is definitely a stretch of an edge case, and there are plenty of ways to track what a thing on the internet was at some point in time.

Who wants this ability? For what reason? Does it outweigh the harm caused by expectations of forever-maintenance?

Again, I know this probably seems like there's some heat behind my position on this, and I know everyone in the WG is trying to do the right thing. But it really sounds like the conversation happened without adequately taking into account the needs of those who've been burned out or struggled to make ends meet while living on the "producer" side of OSS.

Modules and libraries aren't like platforms. They are at times more ephemeral, and more likely to become immortal. For example, the tar module hardly ever gets touched. It's well tested, extremely fast, and very reliable. How long will I be supporting it for? Probably the rest of my career, since I haven't myself fixed a bug in it in over a year. I'll probably spend less than 1 working day on it over the next decade. If I set an expires header, I'll just have to push a new version at some point to update it, making unpaid work for myself (the opposite of the goal of all this) or risk losing the revenue stream from one of my most popular modules.

And if I drop support for it, or decide that I'm too booked up to take on new subscriptions, what then? I have to republish new versions of all my modules? And then, what about people who want to sign up for a support contract who are using a now-unsupported-but-then-supported version? What happens if my subscription management service goes away, because Patreon gets bought by some other company and folded into them?

All of these problems go away if the data is mutable.

@wesleytodd
Copy link
Contributor

But also I'm not sure what solution would require that, where it wouldn't also be in the manifest

if there's a url in package.json, then the registry bundles the support info in manifest and packument requests

If it is in the manifest (which if I understand you correctly I can get from const manifest = await require('pacote').manifest('express'), then I think we are on the same page.

Inaccurate data that leads people to rely on me when they cannot, can only cause hurt feelings and damage my reputation.

Even with a shelf life, an immutable representation of fundamentally mutable data leads inevitably to incorrect data.

Good feedback, and I agree that inaccurate data is a problem, but it is a manageable problem in my experience. The world we are in today has NO DATA on a maintainers intent to support, which is MUCH WORSE. If you can recommend a way to have data and ensure it is always accurate, I am all ears, in my experience this is a "hard problem".

What's the default expires value, if not set?

Oops, we talked about defaulting to 2 years I think but that was not in the draft. I will open a PR to add that.

harm caused by expectations of forever-maintenance?

to provide support for eternity?

I am pretty sure no one has said anything about implied support for eternity. See my comment on expires. The only person I have heard claim they will provide this is @ljharb, and we all think he is heroic for the attempt. I personally know that I will not do this, but if I change my support I am fine publishing a new version of my packages with updated metadata.

Maybe you are not, and I think there is a reason to think like you do. I like your ideas about having a single external source for the metadata as long as it is bundled (as discussed above). So to me it sounds like we are in agreement on the way forward even if we disagree on some details and the meaning behind the metadata.

But it really sounds like the conversation happened without adequately taking into account the needs of those who've been burned out or struggled to make ends meet while living on the "producer" side of OSS.

The group is made up almost entirely of people on the producer side. Please do not disregard the experiences and expertise of people so flippantly. If you would like me to "provide our credentials" I can, but trust me we are not doing this from a place of ignorance.


I am sure I missed responding to some parts, so if you think those are important parts let me know and I can re-read. These are just such long responses it is hard to structure a good response while also attending to and playing with my baby 👶 😄

@isaacs
Copy link
Contributor

isaacs commented Sep 7, 2019

Good feedback, and I agree that inaccurate data is a problem, but it is a manageable problem in my experience. The world we are in today has NO DATA on a maintainers intent to support, which is MUCH WORSE. If you can recommend a way to have data and ensure it is always accurate, I am all ears, in my experience this is a "hard problem".

I think that if it was easy to provide data, then the meaning of "no data" changes from a null to a negative, so this isn't really a fair comparison.

A broken url isn't "no data". It's "this used to be supported, and now isn't." That's meaningful, and most likely accurate in that situation.

The group is made up almost entirely of people on the producer side. Please do not disregard the experiences and expertise of people so flippantly. If you would like me to "provide our credentials" I can, but trust me we are not doing this from a place of ignorance.

My apologies, it was not my intention to dismiss anyone's expertise or credentials, and you're right that was over the line.

Nevertheless, where the conversation landed not only doesn't adequately address the needs of OSS producers, but runs the risk of actually harming them.

Just speaking personally as an open source author, I would not ever feel comfortable using such a system if the data was immutable, and would not recommend it to others. Speaking as an npm founder and executive, I'd have deep ethical reservations about adding any kind of immutably-attached-to-the-artifact support commitments.

Is there a better place for me to register my displeasure with this? Can I join the working group that's writing this spec, and help them do something that's not gonna hurt people? It feels rather orthogonal to this specific pull request.

These are just such long responses it is hard to structure a good response while also attending to and playing with my baby

Totally understand :)

@wesleytodd
Copy link
Contributor

I think that if it was easy to provide data, then the meaning of "no data" changes from a null to a negative, so this isn't really a fair comparison.

I am not sure what you mean here. What I meant with my comment was just that compared to today we are improving things. Do you mean that comparing today to the two future worlds we have proposed is not fair? I can agree with that 😄

My apologies, it was not my intention to dismiss anyone's expertise or credentials, and you're right that was over the line.
Nevertheless, where the conversation landed not only doesn't adequately address the needs of OSS producers, but runs the risk of actually harming them.

Thanks, totally up for criticizing the proposal as long as we are clear that the people working on this are competent, qualified, and well intentioned.

Is there a better place for me to register my displeasure with this? Can I join the working group that's writing this spec, and help them do something that's not gonna hurt people? It feels rather orthogonal to this specific pull request.

Yep, the group is open and we would love your expertise. I think the whole "immutability" stuff here is a bit overplayed at this point. If making this concession is the thing that makes this work I am completely for this. I agree we should move the conversation over to the WG repo, and I am sure the rest of the participants there would love to continue the conversation!

@ljharb
Copy link
Contributor

ljharb commented Sep 8, 2019

The key piece is that this info belongs somewhere that won’t vanish, including historical references of it. If the registry offered a place to put package metadata that wasn’t attached to a version artifact, that could be updated without a full publish, then that seems like it’d be a far better solution than “in a tarball” or “in a separate url that may or may not survive later, or contain historical data”.

Is there any chance the registry might start offering such a feature? That seems like it might address all of our combined concerns pretty well.

@Eomm
Copy link
Contributor

Eomm commented Sep 8, 2019

I think that the main usages of the support are:

  • when a user needs to choose to use or not a module
  • when a user has a problem and needs to find how to ask for help. It is a sync action, so it is online
  • when a user configures a CI tool to check the support value of its dependencies

for this reason, we added to our draft that the source of truth is the metadata of the @latest published version, that is a sort of external URL but based in a robust system that offers the key points that the WG pointed out (sorry if I'm repeating, I'll try to be strict):

  • it not vanish (a repo link could be deleted, if a user delete a package from the registry, the support information in the package itself are meaningless)
  • it can be versioned, so the community can trust packages that don't change the support information every week
  • it is "production-ready": there are not the needs to add a brand new feature to the registry itself

This is the path that brings us here more or less 😃

I'm really glad to read the feedback from @isaacs and I'm asking myself if we could find a solution that is satisfying for all of us, also if npm is a company and needs to follow its decision-making process (I suppose).

Collecting the comments: do you agree with this spec list:

  • the support information may change over time, without the publishing of a new package version
  • the support information must be always in sync, without showing inaccurate data (unless --force 😇)
  • the support information should be versioned
  • the support information is related to the package and not to the repository
  • the user may consume the support info via npm cli or the registry website
    ?

I hope we can find the common ground 💪

@kemitchell
Copy link
Contributor Author

kemitchell commented Sep 9, 2019

Great comments here, but we're back up at a pretty high level. I think @Eomm's and @wesleytodd's comments show the divergent producer- and consumer-centric views of the data we're discussing. But on behalf of the long-suffering producer, I'm still interested in getting something basic and field-upgradable in place sooner rather than later.

#187 patched npm to collect and display support information on each npm install. I closed #187 in favor of this PR, which would patch npm install to display a message encouraging users to run an additional subcommand when any installed package had a support property. The subcommand collects support data by downloading from URLs at support in package.json files and displays them for terminal or as JSON. Minimal impact for consumers there.

For producers, I think it's inferior to postinstall messages for most, at least short-term. But it offers a blessed path for support pleas where there currently is none, plus a better story over the long term, as more and more packages want to ask.

My thought was to make the initial MVP data format as minimal as possible, both to make it easy to use, but then also to take pressure off the WG to rush its work. When the broader-scope spec is fully worked---I believe several new folks, including @isaacs, are now engaged---it should be in a position to offer significant additional value over the starting point proposed here. But in the meantime, the folks behind packages can have a clear voice, a bit of certainty, and reassurance that both npm and the WG are working on their behalf.

Along those lines, I'd be open to pushing a few commits to remove all data fetching and process from the subcommand entirely. For now, projects can set up webpages or files on master to point to the relevant places for all the relevant people, and past those URLs into package.json. npm support can print a report for terminal or as JSON with no network requests whatsoever. The URLs can resolve or redirect or negotiate content-type ... whatever the maintainers care to do.

In the future, as WG and registry particulars stabilize, the CLI would be in a good position to encourage updates to a richer package.json schema, via warning messages. We've seen those work well for licensing metadata, and I think they might work even better for support/backing data. Of course, the CLI would also be in a good position to help users adapt should the registry start sending or hosting data records on producers' behalf.

Prior Art: feross/thanks#2. There is nothing new under the sun.

doc/files/support.json Outdated Show resolved Hide resolved
@kemitchell
Copy link
Contributor Author

I've pushed commits paring this PR down to the absolute bare bones. All it does is look for URLs in support projects. If present, npm install will mention running npm support. In turn, npm support will print a simple mapping of packages to support URLs.

@mhdawson
Copy link

@isaacs I think it would be good to move the conversation to nodejs/package-maintenance#241 in order to avoid hijacking this PR, but I do want to highlight that https://github.com/nodejs/package-maintenance/blob/master/docs/drafts/PACKAGE-SUPPORT.md is still considered a draft (and in the drafts directory) specifically because we are still want/need broader input/discussion on the most contentious issues (which the location of the data/immutability has been one throughout the discussions). WIll be great to have you jump into that discussion.

doc/files/package.json.md Outdated Show resolved Hide resolved
You can specify a URL for up-to-date information about ways to support
development of your package:

{ "support": "https://example.com/project/support" }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth versioning this attribute so we can make changes to it in the future? It would allow us to add complexity over time as we see the need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a string URL, it's a string URL. We don't get to version that standard. :-P

If it's not a string URL, it could be an object with a version property ... if/when we go there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. The latter is what I was thinking.

{
  "support": {
    "version": "1",
    "url": "https://example.com/project/support"
  }
}

But also it might be too weird to version individual attributes rather than the package.json schema as a whole.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does collide with what we are defining in https://github.com/nodejs/package-maintenance/blob/master/docs/drafts/PACKAGE-SUPPORT.md.

It would be good to avoid a conflict with that in that we'll be suggesting package maintainers add the larger structure.

Would what is suggested in maintenance/blob/master/docs/drafts/PACKAGE-SUPPORT.md be possible if qualified through a later version? And if so how would we have to update what is in there if we wanted to suggest the larger structure start to be used today? Alternatively, we could change "support" to something else but then it would potentially duplicate info.

@kemitchell
Copy link
Contributor Author

kemitchell commented Oct 2, 2019 via email

ruyadorno pushed a commit to ruyadorno/cli that referenced this pull request Oct 31, 2019
PR-URL: npm#246
Credit: @kemitchell
Close: npm#246
Reviewed-by: @ruyadorno

Thanks @kemitchell for providing the initial work that served as a base
for `npm fund`, its original commits messages are preserved as such:

- support: add support subcommand
- support: fix request caching
- support: further sanitize contributor data
- doc: Fix typo
- support: simplify to just collecting and showing URLs
- install: improve `npm support` test
- install: drop "the" before "projects you depend on"
- doc: Reword mention of `npm support` in `package.json` spec
@ruyadorno ruyadorno closed this in 266d076 Nov 5, 2019
@ljharb
Copy link
Contributor

ljharb commented Nov 5, 2019

@ruyadorno it's unfortunate this was merged and included, as it now collides with what the package management node group has been working on. What made this ready to land?

@ruyadorno
Copy link
Contributor

ruyadorno commented Nov 5, 2019

hi @ljharb worry not, this was merged as foundational work for what became npm fund - I just wished to credit @kemitchell (given that I used his work as a starting point) so I did preserved his contribution in the git history but what ended up landing on v6.13.0 is only npm fund

@ljharb
Copy link
Contributor

ljharb commented Nov 5, 2019

Thanks for the clarification!

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.

10 participants