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.

Sign up for GitHub

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

Jump to bottom

feat: add npm set-script #2237

Merged
merged 1 commit into from
Dec 4, 2020
Merged

feat: add npm set-script #2237

merged 1 commit into from
Dec 4, 2020

Conversation

Yash-Singh1
Copy link
Contributor

Summary

This PR introduces the set-script command. It accepts two arguments, the script name and the command as said in RFC 0016

References

Adds in RFC 0016: https://github.com/npm/rfcs/blob/latest/accepted/0016-set-script-command.md

@Yash-Singh1 Yash-Singh1 requested a review from a team as a code owner November 24, 2020 22:27
@ljharb
Copy link
Contributor

ljharb commented Nov 24, 2020

What happens if a package uses npm set-strict in a postinstall script? Should perhaps this command explicitly fail in that scenario? (I realize that a package can probably already do this, but this command might make it easier)

@Yash-Singh1
Copy link
Contributor Author

What happens if a package uses npm set-strict in a postinstall script? Should perhaps this command explicitly fail in that scenario? (I realize that a package can probably already do this, but this command might make it easier)

Any suggestions on how to detect or identify if it is being called from postinstall?

@ruyadorno
Copy link
Contributor

What happens if a package uses npm set-strict in a postinstall script? Should perhaps this command explicitly fail in that scenario? (I realize that a package can probably already do this, but this command might make it easier)

Right, I've read through the original RFC discussion to find that there was no definitive stance on it so it didn't make it to the RFC itself - I'm ok with that extra check being there since it's unlikely to be an issue to 99% of users.

Any suggestions on how to detect or identify if it is being called from postinstall?

You can check for process.env.npm_lifecycle_event === 'postinstall' and throw an error then 😊

@ruyadorno ruyadorno added pr: needs tests requires tests before merging Release 7.x work is associated with a specific npm 7 release pr: needs documentation pull request requires docs before merging labels Nov 25, 2020
ruyadorno
ruyadorno reviewed Nov 25, 2020
View reviewed changes
Copy link
Contributor

@ruyadorno ruyadorno 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 so much @Yash-Singh1 for picking this up from the RFCs and turning it into actual code 😄 This is looking very good, I think it just needs to adjust some details I left in the review + addressing lifecycle check from @ljharb 😊

For tests and docs you can refer to other commands for examples but definitively let us know if you need help/guidance. Thanks again! 🎉

lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
ljharb
ljharb reviewed Nov 25, 2020
View reviewed changes
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This doesn't seem like it has actual tests?

lib/set-script.js Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
ruyadorno
ruyadorno reviewed Nov 26, 2020
View reviewed changes
lib/set-script.js Outdated Show resolved Hide resolved
ljharb
ljharb reviewed Nov 26, 2020
View reviewed changes
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

seems good to me, but i don't see any unit tests.

lib/set-script.js Outdated Show resolved Hide resolved
@darcyclarke darcyclarke added the semver:minor new backwards-compatible feature label Nov 28, 2020
@Yash-Singh1
Copy link
Contributor Author

@ljharb @ruyadorno I am ready to merge from my end.

ruyadorno
ruyadorno reviewed Dec 1, 2020
View reviewed changes
Copy link
Contributor

@ruyadorno ruyadorno left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some very minor suggestions to polish things up even more but otherwise we're looking good to ship it! :shipit:

docs/content/commands/npm-set-script.md Outdated Show resolved Hide resolved
docs/content/commands/npm-set-script.md Show resolved Hide resolved
docs/content/commands/npm-set-script.md Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
lib/set-script.js Outdated Show resolved Hide resolved
test/lib/set-script.js Outdated Show resolved Hide resolved
test/lib/set-script.js Outdated Show resolved Hide resolved
test/lib/set-script.js Outdated Show resolved Hide resolved
test/lib/set-script.js Outdated Show resolved Hide resolved
@ruyadorno ruyadorno added release: next These items should be addressed in the next release and removed pr: needs documentation pull request requires docs before merging pr: needs tests requires tests before merging labels Dec 1, 2020
ruyadorno
ruyadorno reviewed Dec 1, 2020
View reviewed changes
test/lib/set-script.js Outdated Show resolved Hide resolved
@ruyadorno ruyadorno changed the title Added in set-script feat: add npm set-script Dec 1, 2020
@Yash-Singh1 @ruyadorno
feat: add npm set-script
6b15751 
Introduces the set-script command. It accepts two arguments,
the script name and the command

ref: https://github.com/npm/rfcs/blob/latest/accepted/0016-set-script-command.md

PR-URL: #2237
Credit: @Yash-Singh1
Close: #2237
Reviewed-by: @ruyadorno
@ruyadorno ruyadorno changed the base branch from latest to release/v7.1.0 December 4, 2020 19:23
@ruyadorno ruyadorno merged commit 6b15751 into npm:release/v7.1.0 Dec 4, 2020
This was referenced Dec 4, 2020
Merged
Closed
@MylesBorins MylesBorins mentioned this pull request Dec 18, 2020
Closed
@Jankyboy Jankyboy mentioned this pull request Feb 18, 2021
Open
@thebpc thebpc mentioned this pull request Feb 19, 2021
Open
@snyk-bot snyk-bot mentioned this pull request May 10, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@ruyadorno ruyadorno ruyadorno approved these changes

Assignees
No one assigned
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yash-Singh1 @ljharb @ruyadorno @darcyclarke