-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
docs: Added steps for using npm/npx locally to CONTRIBUTING.md #6643
Conversation
These suggestions seem quite overly prescriptive. The folder where things are checked out will vary by user (and in the case of git worktrees, with every branch) so aliases don't seem like the best suggestion.
|
CONTRIBUTING.md
Outdated
For example instead of: | ||
```bash | ||
npx --no-install esbuild | ||
npm install esbuild |
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.
this is not an analog for npx
. You'd want to run the bin directly or node . exec
CONTRIBUTING.md
Outdated
``` | ||
Use: | ||
```bash | ||
localnpx --no-install esbuild | ||
node . install esbuild |
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.
same here, install is not the same as npx/npm exec
Appreciate the feedback, we made the appropriate changes. We thought that having an npm Please let us know if there's anything else that we should change. |
CONTRIBUTING.md
Outdated
@@ -66,11 +66,11 @@ node . exec | |||
|
|||
For example instead of: | |||
```bash | |||
npm install esbuild | |||
npm exec --yes false -- esbuild |
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.
What's the thinking behind forcing yes
to be false?
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.
Forcing yes
to be false is not really necessary, as this is just an example command. We included it because it was a reference to the last issue we worked on.
Would you prefer if we made the command broader and changed it to: npm exec -- <package>
and node . exec -- <package>
?
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.
Yeah suggestions, especially in readmes etc, should be as minimally prescriptive as possible. Folks are going to be copying and pasting it, and anything we bake in that they may not need by default is potentially confusing to them. Especially in this case the suggestion would fail for most folks who don't already have esbuild in their npx cache.
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.
So yes, let's make it broader, exactly like your updated suggestions.
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.
Just pushed those changes. Appreciate the explanation, that makes a lot of sense!
CONTRIBUTING.md
Outdated
@@ -48,6 +48,32 @@ We use [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/). | |||
|
|||
We use [`tap`](https://node-tap.org/) for testing & expect that every new feature or bug fix comes with corresponding tests that validate the solutions. Tap also reports on code coverage and it will fail if that drops below 100%. | |||
|
|||
## Run npm/npx Locally | |||
|
|||
If a specific command is not covered by tap, try the following: |
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 this line can be removed too. The advice is good for general testing if anything you're doing locally when developing, even long before you get to tests. Everything else looks good!
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.
Okay, just removed that section, thanks for all your help!
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.
Thank you
Summary
We noticed there were no steps for using npm/npx locally on the CONTIBUTING.md file so we decided to add a new section that explained how to do this.