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

libnpmexec: allow custom stdio option value. #6976

Open
wants to merge 3 commits into
base: latest
Choose a base branch
from
+59 −2
Open

libnpmexec: allow custom stdio option value. #6976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93fb751
add stdio option
mshima Nov 8, 2023
e7ff14d
add stdio option
mshima Nov 8, 2023
e4cd2c6
add test
mshima Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions workspaces/libnpmexec/lib/index.js
View file
Open in desktop
Expand Up @@ -90,6 +90,7 @@ const exec = async (opts) => {
path = '.',
runPath = '.',
scriptShell = isWindows ? process.env.ComSpec || 'cmd' : 'sh',
stdio,
...flatOptions
} = opts

Expand All @@ -104,6 +105,7 @@ const exec = async (opts) => {
binPaths,
runPath,
scriptShell,
stdio,
})

// interactive mode
Expand Down
7 changes: 5 additions & 2 deletions workspaces/libnpmexec/lib/run-script.js
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@ const run = async ({
binPaths,
runPath,
scriptShell,
stdio = 'inherit',
}) => {
// turn list of args into command string
const script = call || args.shift() || scriptShell
Expand Down Expand Up @@ -60,11 +61,13 @@ const run = async ({
binPaths,
event: 'npx',
args,
stdio: 'inherit',
stdio,
scriptShell,
})
} finally {
npmlog.enableProgress()
if (stdio === 'inherit') {
Copy link
Member

Choose a reason for hiding this comment

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

this one needs to be removed also

Copy link
Author

Choose a reason for hiding this comment

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

The idea is to make the libnpmexec don’t output npm logs.
npm log output is interlacing with the output of the application executing libnpmexec.
Just passing the stdio value through is not enough.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't npm log output, this is progress bars specifically.

To suppress all logging we need npmlog.pause and npmlog.resume, but these should be independent of enabling/disabling progress.

Copy link
Author

Choose a reason for hiding this comment

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

I don't want to install npmlog directly to pause it.
What about a flag to ignore npmlog?

Copy link
Member

Choose a reason for hiding this comment

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

You already have it

cli/workspaces/libnpmexec/lib/run-script.js

Line 4 in e4cd2c6

const npmlog = require('npmlog')

Copy link
Author

Choose a reason for hiding this comment

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

I mean, I have ora as progress bar.
npmlog and ora are mangling the output.
I need to install and import npmlog in my project to pause the npmlog.
And that does not guarantee that the npmlog is muted due to hoisting.

npmlog.enableProgress()
}
}
}

Expand Down
14 changes: 14 additions & 0 deletions workspaces/libnpmexec/test/index.js
View file
Open in desktop
Expand Up @@ -14,3 +14,17 @@ t.test('no args', async t => {

await exec()
})

t.test('stdio value', async t => {
t.plan(1)

const { exec } = setup(t, {
mocks: {
'../../lib/run-script': ({ stdio }) => {
t.equal(stdio, 'pipe', 'should use expected stdio value')
},
},
})

await exec({ stdio: 'pipe' })
})
38 changes: 38 additions & 0 deletions workspaces/libnpmexec/test/run-script.js
View file
Open in desktop
Expand Up @@ -141,3 +141,41 @@ t.test('ci env', async t => {

await runScript()
})

t.test('stdio value', t => {
t.test('stdio default value', async t => {
const runScript = await mockRunScript(t, {
'@npmcli/run-script': (opt) => {
t.equal(opt.stdio, 'inherit', 'should use expected stdio value')
},
npmlog: {
disableProgress () {
t.ok('should disable progress')
},
enableProgress () {
t.ok('should enable progress')
},
},
})
await runScript({ args: [], runPath: t.testDirName })
})

t.test('stdio custom value', async t => {
const runScript = await mockRunScript(t, {
'@npmcli/run-script': (opt) => {
t.equal(opt.stdio, 'pipe', 'should use expected stdio value')
},
npmlog: {
disableProgress () {
t.ok('should disable progress')
},
enableProgress () {
t.fail('should not enable progress')
},
},
})
await runScript({ args: [], stdio: 'pipe', runPath: t.testDirName })
})

t.end()
})