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 fund subcommand #273

Closed
wants to merge 5 commits into from

Conversation

@ruyadorno
Copy link
Member

ruyadorno commented Oct 31, 2019

Overview

This PR introduces the npm fund command that lists all funding info provided by the installed dependencies of a given project.

✏️ Changes

  • lib/utils/funding.js Provides helpers to validate funding info and
    return a tree-shaped structure containing the funding data for all deps.
  • lib/fund.js Implements npm fund <pkg> command
  • Added tests
    • npm install mention of funding
    • npm fund <pkg> variations
    • unit tests for added lib/utils and lib/install helpers
  • Added docs for npm fund, funding package.json property
  • Fixed lib/utils/open-url to support --json config
  • Documented unicode on npm install docs

🔗 References

📸 Screenshots

Simple npm install within a project with given dependencies

Demo showing simple npm install within a project with given dependencies

npm install a package that contains dependencies with funding declared, fires up npm ls and npm fund to show differences in output

Demo showing npm install a package that contains dependencies with funding declared, fires up npm ls and npm fund to show differences

npm fund --json output example

Demo showing npm fund --json example

🔍 Testing

Install a package that has a valid funding object declared on its package.json, e.g:

  • Single package: npm install sleepover
  • Many nested packages with funding info: npm install @ruyadorno/test-funding-dep

This change has unit test coverage
This change has integration test coverage

🔥 Rollback

If we observe something wrong with this change in production, how should we respond?

This can be reverted at any time

kemitchell and others added 2 commits Aug 30, 2019
PR-URL: #246
Credit: @kemitchell
Close: #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
This commit introduces the `npm fund` command that lists all `funding`
info provided by the installed dependencies of a given project.

Notes on implementation:

- `lib/utils/funding.js` Provides helpers to validate funding info and
return a tree-shaped structure containing the funding data for all deps.
- `lib/fund.js` Implements `npm fund <pkg>` command
- Added tests
  - `npm install` mention of funding
  - `npm fund <pkg>` variations
  - unit tests for added `lib/utils` and `lib/install` helpers
- Added docs for `npm fund`, `funding` `package.json` property
- Fixed `lib/utils/open-url` to support `--json` config
- Documented `unicode` on `npm install` docs

Refs: https://github.com/npm/rfcs/blob/2d2f00457ab19b3003eb6ac5ab3d250259fd5a81/accepted/0017-add-funding-support.md
@ruyadorno ruyadorno requested a review from npm/cli-team as a code owner Oct 31, 2019
@brodybits

This comment has been minimized.

Copy link

brodybits commented Oct 31, 2019

I would personally favor some way to advertise which project maintainers are available for hire with some quick details or have any other services to offer (no third-party ads please).

For example (true example): "@brodybits is available for short-term hire in Boston, NYC, or remote contract"

@darcyclarke

This comment has been minimized.

Copy link
Member

darcyclarke commented Oct 31, 2019

@brodybits Hey Chris! Appreciate the feedback but I think it's out of the scope of this work which was talked about & simplified pretty extensively in the original RFC. If you have an idea about a future feature/design we could support, definitely feel free to contribute an RFC over on our npm/rfcs repo and/or, where I think your suggestion may make the most sense, is to investigate the work the Node Foundation Package Maintenance Working Group has done around a more verbose support schema.

@giuseppeg

This comment has been minimized.

Copy link

giuseppeg commented Nov 1, 2019

This looks amazing! It would be nice to have the info deduplicated and flattened and maybe have an advanced mode (maybe a --tree flag) to show how the packages are related.

There seem to be a lot of "noise" in the output from the first gif, a flat list with a white line to break every package info would be more readable.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Nov 1, 2019

If i haven’t chosen to add a dep to package.json, it’s critical that i know how and why a dep is in my graph, or else why would i ever want to give it money?

@giuseppeg

This comment has been minimized.

Copy link

giuseppeg commented Nov 1, 2019

@ljharb fair but imagine having 50 packages with sub deps. The list could still be flat and each field can say "Used in: babel-core" or something (assuming babel-core is the dependency you installed).

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Nov 1, 2019

it could, but presumably you’d want the list sorted, with direct deps at the top, and then transitive deps sorted by “used in the most packages”. (iow, I’m partially agreeing with you; a tree isn’t the best way to represent this since duplicated deps should be consolidated and summarized)

@giuseppeg

This comment has been minimized.

Copy link

giuseppeg commented Nov 1, 2019

Lock files (yarn.lock and package-lock.json) are flat, I'd personally go for that. That said probably shipping any solution and asking the community for feedback is the best way to go.

@coreyfarrell

This comment has been minimized.

Copy link

coreyfarrell commented Nov 1, 2019

A potential issue is that this is version based rather than package based? This means that I cannot set funding information for already released versions of a package. More importantly if funding information changes after a release I cannot fix it (remove a no longer valid funding method for example).

@ruyadorno

This comment has been minimized.

Copy link
Member Author

ruyadorno commented Nov 1, 2019

@npm/cli-team: @giuseppeg raised an important point that went a bit under documented in my original description which is that the current implementation of npm fund output does some smart things when building that tree, such as moving transitive deps up in the tree if their parents has no funding info (since it only displays packages that contain such info) along with the stacking of same-maintainer info demoed in the 2nd gif there.

I quickly put together a demo showing how childs will show up a level above compared to a npm ls output:

in this demo the dependency@ruyadorno/dep-sub-no-funding@1.0.0 which has no funding info is removed from the npm fund listing and its child are listed as children of its parent

Demo showing diff between npm ls and npm fund when a parent dep has no funding info

the takeaway here is that this output is neither an exact representation of the dependency tree nor a flat list, it's something in between and my idea when working on it is that it's a middle-ground to start with and we can iterate later on with feedback from the community

@brodybits

This comment has been minimized.

Copy link

brodybits commented Nov 1, 2019

it’s critical that i know how and why a dep is in my graph

@ljharb yarn why would do that if you use Yarn.

I think this should be supported by npm, looks like there is already a npm-why package to do this.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Nov 1, 2019

@brodybits i don't need to know that information in general, and i know how to get it without using yarn - i'm talking about directly inline in the funding info, which is where i'd need to see it to gain motivation to donate money to a transitive dep.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Nov 1, 2019

@ruyadorno and what if test-funding-dep-sub-sub-sub is indirectly used by 23 different top-level packages? must i see it listed 23 times, or must its provenance only be shown for one ancestor and the other 22 lost?

@ruyadorno

This comment has been minimized.

Copy link
Member Author

ruyadorno commented Nov 1, 2019

or must its provenance only be shown for one ancestor and the other 22 lost?

☝️ that is the case yes, in its current implementation

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Nov 1, 2019

That seems like proof that this is not the optimal output format to use.

@znarf znarf referenced this pull request Nov 5, 2019
1 of 3 tasks complete
// within top levels takes precedence over nested ones
return (Object.keys(deps)).map((key) => {
const dep = deps[key]
const { name, funding, version } = dep

This comment has been minimized.

Copy link
@znarf

znarf Nov 5, 2019

Suggested change
const { name, funding, version } = dep
const { name, version } = dep
const funding = dep.funding || dep.collective;

There are many packages using the collective property today to expose their Open Collective URL. Would be great to support that when funding is not found, while we wait for them to add it. I

The syntax of collective is exactly the same as funding, making the implementation uncomplicated.

It's also nice that this allow anyone to test the feature and see results today, while in the current state it's giving me only empty results for all the projects I tested.

Community & Open Source automation moved this from In Review to Done Nov 5, 2019
@curbengh curbengh referenced this pull request Nov 10, 2019
0 of 2 tasks complete
aladdin-add added a commit to eslint/eslint that referenced this pull request Nov 10, 2019
npm just added a new npm fund command in it's v6.13.0 release as part of the efforts to help out the OSS community.

refs:npm/cli#273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants
You can’t perform that action at this time.