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: allow script sort for run-p #240

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brodyd795
Copy link

@brodyd795 brodyd795 commented Jan 21, 2022

Background

This package does not currently support sorting the scripts field of a package.json when npm-run-all is a devDependency, regardless of how it is actually used in the scripts.

While there is certainly a place for refraining from sorting the scripts when npm-run-all is used for running scripts sequentially (e.g. npm-run-all --serial), there are use cases where the user may wish for the scripts to be sorted even when npm-run-all is a devDepenency (e.g. npm-run-all -p). This concern has been raised in previous PRs and issues (see this PR and this issue).

This PR enables this behavior.

Changes

The following uses of npm-run-all will continue to NOT sort the scripts:

  • npm-run-all
  • npm-run-all --serial
  • npm-run-all --sequential
  • npm-run-all -s
  • run-s

While the following uses now WILL sort the scripts:

  • npm-run-all --parallel
  • run-p

@brodyd795 brodyd795 changed the title fix: allow sort for run-s feat: allow sort for run-p Jan 21, 2022
@brodyd795 brodyd795 changed the title feat: allow sort for run-p feat: allow script sort for run-p Jan 21, 2022
@MichaelDeBoey
Copy link
Contributor

@keithamus Is there anything I can do to help in order to get this one merged?

@keithamus
Copy link
Owner

Sure! I hadn't noticed this PR before as it's a draft and so didn't end up in my inbox. So to start I'd say taking this PR and making it a non-draft PR would be a good first step!

@MichaelDeBoey
Copy link
Contributor

taking this PR and making it a non-draft PR would be a good first step!

@brodyd795 Can we set this PR 'ready for review' please?

Comment on lines +145 to +162
if (hasDevDependency('npm-run-all', packageJson)) {
const scripts = packageJson.scripts
const betterScripts = packageJson.betterScripts

if (scripts) {
return Object.values(scripts).some((script) =>
runSValues.some((runSValue) => runSValue.test(script)),
)
}

if (betterScripts) {
return Object.values(betterScripts).some((script) =>
runSValues.some((runSValue) => runSValue.test(script)),
)
}
}

return false
Copy link
Contributor

Choose a reason for hiding this comment

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

What about an early return here?

Suggested change
if (hasDevDependency('npm-run-all', packageJson)) {
const scripts = packageJson.scripts
const betterScripts = packageJson.betterScripts
if (scripts) {
return Object.values(scripts).some((script) =>
runSValues.some((runSValue) => runSValue.test(script)),
)
}
if (betterScripts) {
return Object.values(betterScripts).some((script) =>
runSValues.some((runSValue) => runSValue.test(script)),
)
}
}
return false
if (!hasDevDependency('npm-run-all', packageJson)) {
return false
}
const scripts = packageJson.scripts
const betterScripts = packageJson.betterScripts
if (scripts) {
return Object.values(scripts).some((script) =>
runSValues.some((runSValue) => runSValue.test(script)),
)
}
if (betterScripts) {
return Object.values(betterScripts).some((script) =>
runSValues.some((runSValue) => runSValue.test(script)),
)
}

Comment on lines +189 to +191
const toReturn = sortObjectKeys(scripts, order)

return sortObjectKeys(scripts, order)
return toReturn
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're only using toReturn as a return value, I think we can just keep it as it was?

Suggested change
const toReturn = sortObjectKeys(scripts, order)
return sortObjectKeys(scripts, order)
return toReturn
return sortObjectKeys(scripts, order)

@keithamus
Copy link
Owner

@MichaelDeBoey given this PR is coming on for a year old, you might consider forking it and making a new PR. You can keep @brodyd795's commits in, which means you'll both get contribution credit!

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