Skip to content

Conversation

AaronHamilton965
Copy link
Contributor

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.

@AaronHamilton965 AaronHamilton965 requested a review from a team as a code owner July 13, 2023 22:13
@wraithgar
Copy link
Member

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.

node . and node . exec are the preferred ways to run npm and npm exec when working locally.

CONTRIBUTING.md Outdated
For example instead of:
```bash
npx --no-install esbuild
npm install esbuild
Copy link
Member

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
Copy link
Member

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

@rahulio96
Copy link
Contributor

Appreciate the feedback, we made the appropriate changes.

We thought that having an npm install command instead of exec as an example would clear up confusion when potential contributors see node . <command> at line 59. However, we agree with you that having the exec command there is the better alternative.

Please let us know if there's anything else that we should change.

CONTRIBUTING.md Outdated
For example instead of:
```bash
npm install esbuild
npm exec --yes false -- esbuild
Copy link
Member

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?

Copy link
Contributor

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> ?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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

## Run npm/npx Locally

If a specific command is not covered by tap, try the following:
Copy link
Member

@wraithgar wraithgar Jul 17, 2023

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!

Copy link
Contributor

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!

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

Thank you

@wraithgar wraithgar merged commit 36bf5fe into npm:latest Jul 17, 2023
@github-actions github-actions bot mentioned this pull request Jul 17, 2023
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.

3 participants