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

fix(fund): support funding string shorthand #472

Closed
wants to merge 1 commit into from

Conversation

ruyadorno
Copy link
Collaborator

Overview

In the approved RFC it was documented that npm fund should also support a shorthand version of the funding property using only a string instead of an object.

✏️ Changes

  • Fixes funding behavior to normalize a string argument into valid info
  • Add tests to make sure everything works as expected

🔗 References

🔍 Testing

Manual testing:
In a folder with a given package.json:

{
  "name": "foo",
  "version": "1.0.0",
  "funding": "http://example.com"
}

Running npm fund should output:

$ npm fund
foo@1.0.0
└── url: http://example.com

✅ 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

In the approved RFC it was documented that `npm fund` should also
support a shorthand version of the `funding` property using only a
string instead of an object.

This commit fixes it and adds tests to ensure its behavior.
@ruyadorno ruyadorno requested a review from a team as a code owner November 11, 2019 20:45
@ruyadorno ruyadorno added this to In Review in Community & Open Source Nov 11, 2019
@ljharb
Copy link
Collaborator

ljharb commented Nov 12, 2019

I tried this and discovered it didn't work.

Also, what about multiple values? Should/could it accept an array too?

@claudiahdz claudiahdz added Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes labels Nov 12, 2019
@ruyadorno
Copy link
Collaborator Author

Also, what about multiple values? Should/could it accept an array too?

@ljharb multiple values are outside of the scope of the approved RFC 😬

claudiahdz pushed a commit that referenced this pull request Nov 13, 2019
In the approved RFC it was documented that `npm fund` should also
support a shorthand version of the `funding` property using only a
string instead of an object.

This commit fixes it and adds tests to ensure its behavior.

PR-URL: #472
Credit: @
Close: #472
Reviewed-by: @claudiahdz
@ljharb
Copy link
Collaborator

ljharb commented Nov 13, 2019

Gotcha - given that packages have multiple authors, and a single author can have multiple funding options (i have GitHub sponsors and tidelift, for example) - it’d be great to add that. I’ll try to find time to write an RFC, if someone doesn’t beat me to it.

@ruyadorno
Copy link
Collaborator Author

oh yeah 🙏 please do, btw we have the OpenRFC call happening today

Copy link
Contributor

@mikemimik mikemimik left a comment

Choose a reason for hiding this comment

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

small nit

LGTM!

function retrieveFunding (funding) {
return typeof funding === 'string'
? {
url: funding
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would change this syntax to a one liner; { url: funding }. When I read this now it makes me think it's a block scope and something interesting is happening.

Community & Open Source automation moved this from In Review to Approved Nov 13, 2019
@claudiahdz claudiahdz mentioned this pull request Nov 18, 2019
Community & Open Source automation moved this from Approved to Done Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants