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

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
Collaborator

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
Collaborator

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

@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
Copy link
Collaborator

@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
lib/set-script.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Copy link
Collaborator

@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.

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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants