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

Use npm-run-all for danger:prepush to run scripts in sequence #1254

Closed
wants to merge 2 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Mar 16, 2022

Noticed that "danger:prepush" ran commands with ";" rather "&&" meaning the next command is still executed if the previous one failed. This resulted in wery weird errors due to the previous step failure.

This PR uses "npm-run-all" module for platform agnostic and clean API support:

Copy link
Member

@fbartho fbartho left a comment

Choose a reason for hiding this comment

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

I commented in #1252 not realizing this PR existed.

Sorry, I’d prefer using the && syntax instead of pulling a new dependency for this purpose.
This change adds extra maintenance burden without enough benefit in my opinion.

@glensc
Copy link
Contributor Author

glensc commented Mar 17, 2022

I find "&&" less readable; also it doesn't work on all platforms. Up to maintainers of course what platforms they want to support for development.

npm-run-all also has "build:*" syntax to automatically run all jobs matching, but then the ordering would be incorrect.

the other way to solve this is with dependencies. i.e each script can have "pre" script, i.e if you run "build:bla" then "prebuild:bla" is checked and ran.

I'm not sure if this is yarn thing, or npm does it too.

@fbartho
Copy link
Member

fbartho commented Mar 17, 2022

I do understand that some shell syntaxes are maybe foreign for some people. That said, using && is a classic shell rule so shouldn’t be considered “uncommon” or “strange” if you’re writing a CLI command-string. I have no idea what OS doesn’t support this, but we haven’t had any complaints or Feature requests related to such a thing.

This repo has a small-to-medium amount of shell commands, and this PR is only “helping” one of those commands. As a result, I’m just not convinced that the benefit outweighs the maintenance burden of a new dependency.

Additionally, looking at the tool’s pages on npmJS and Github, it hasn’t had a release in 3 years, it “supports” node ^4.9.1. Adopting this dependency would immediately cause maintenance problems. That alone is enough to disqualify this tool even if we had a significant need for it.

@glensc
Copy link
Contributor Author

glensc commented Mar 17, 2022

What I've heard is that windows platforms don't do POSIX shell syntax. I have no way to verify this.

but the original problem is that the prepush pipeline doesn't abort on the first error, shall I provide PR with using && then?

@fbartho
Copy link
Member

fbartho commented Mar 18, 2022

I would be happy to review/approve a PR that solves it without adding a dependency if you want to contribute it! Thanks for understanding @glensc!

@glensc
Copy link
Contributor Author

glensc commented Mar 18, 2022

Replacement: #1258

@glensc glensc closed this Mar 18, 2022
@glensc glensc deleted the npm-run-all branch March 18, 2022 08:06
@glensc
Copy link
Contributor Author

glensc commented Mar 29, 2022

@fbartho can you review/approve (and merge?) #1258 then?

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.

2 participants