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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,29 @@ const hasDevDependency = (dependency, packageJson) => {
)
}

const runSValues = [new RegExp('run-s')]

const hasRunS = (packageJson) => {
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
Comment on lines +145 to +162
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)),
)
}

}

const sortScripts = onObject((scripts, packageJson) => {
const names = Object.keys(scripts)
const prefixable = new Set()
Expand All @@ -152,7 +175,7 @@ const sortScripts = onObject((scripts, packageJson) => {
return name
})

if (!hasDevDependency('npm-run-all', packageJson)) {
if (!hasRunS(packageJson)) {
keys.sort()
}

Expand All @@ -163,8 +186,9 @@ const sortScripts = onObject((scripts, packageJson) => {
),
[],
)
const toReturn = sortObjectKeys(scripts, order)

return sortObjectKeys(scripts, order)
return toReturn
Comment on lines +189 to +191
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)

})

// fields marked `vscode` are for `Visual Studio Code extension manifest` only
Expand Down
60 changes: 49 additions & 11 deletions tests/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@ const fixture = {
'pre-fetch-info': 'foo',
}

const fixtureWithRunS = {
test: 'node test.js',
multiply: '2 * 3', // between p(ostinstall) and install
watch: 'watch things',
prewatch: 'echo "about to watch"',
postinstall: 'echo "Installed"',
preinstall: 'echo "Installing"',
start: 'node server.js',
posttest: 'run-s abc def',
pretest: 'xyz',
postprettier: 'echo "so pretty"',
preprettier: 'echo "not pretty"',
prettier: 'prettier -l "**/*.js"',
prepare: 'npm run build',
'pre-fetch-info': 'foo',
}

const expectAllSorted = {
preinstall: 'echo "Installing"',
postinstall: 'echo "Installed"',
Expand All @@ -38,7 +55,7 @@ const expectAllSorted = {
const expectPreAndPostSorted = {
pretest: 'xyz',
test: 'node test.js',
posttest: 'abc',
posttest: 'run-s abc def',
multiply: '2 * 3',
prewatch: 'echo "about to watch"',
watch: 'watch things',
Expand All @@ -53,18 +70,39 @@ const expectPreAndPostSorted = {
}

for (const field of ['scripts', 'betterScripts']) {
test(`${field} when npm-run-all is not a dev dependency`, macro.sortObject, {
test(`${field} when npm-run-all is NOT a dev dependency`, macro.sortObject, {
value: { [field]: fixture },
expect: { [field]: expectAllSorted },
})
test(`${field} when npm-run-all is a dev dependency`, macro.sortObject, {
value: {
[field]: fixture,
devDependencies: { 'npm-run-all': '^1.0.0' },
},
expect: {
[field]: expectPreAndPostSorted,
devDependencies: { 'npm-run-all': '^1.0.0' },

for (const type of ['run-s']) {
test(
`${field} when npm-run-all IS a dev dependency, and IS used in scripts in form of ${type}`,
macro.sortObject,
{
value: {
[field]: fixtureWithRunS,
devDependencies: { 'npm-run-all': '^1.0.0' },
},
expect: {
[field]: expectPreAndPostSorted,
devDependencies: { 'npm-run-all': '^1.0.0' },
},
},
)
}
test(
`${field} when npm-run-all IS a dev dependency, but is NOT used in scripts`,
macro.sortObject,
{
value: {
[field]: fixture,
devDependencies: { 'npm-run-all': '^1.0.0' },
},
expect: {
[field]: expectAllSorted,
devDependencies: { 'npm-run-all': '^1.0.0' },
},
},
})
)
}