Fix fd-specific options to account for ipc#1241
Conversation
Keep numeric fd-specific options separate from the internal IPC slot by validating fd names against the real stdio length while reserving a distinct pseudo-slot for ipc. Update result types so ipc-specific options do not affect numeric fd lookups, and add runtime and tsd regressions for maxBuffer, buffer, verbose, and result stdio typing. Fixes #1239
sindresorhus
force-pushed
the
ss/investigate-issue-1239
branch
from
5e5d9c1 to
924c488
Compare
|
@ehmicky Going to merge some PRs right away to get things done, but if you have time, a review is always welcome, but I don't want to wait or stress you about it. |
|
|
||
| test('Strips color sequences from stdout', async t => { | ||
| const {stderr} = await nestedSubprocess('noop.js', [foobarRed], {verbose: 'full'}); | ||
| const {stderr} = await nestedSubprocess('noop.js', [red(foobarString)], {verbose: 'full'}, {env: {FORCE_COLOR: '1', NO_COLOR: undefined}}); |
There was a problem hiding this comment.
It appears that some of this PR is accidentally reverting #1242?
I.e. it re-introduces yoctocolors in the tests and removes the usage of the foobarRed variable.
|
|
||
| setFixtureDirectory(); | ||
|
|
||
| const redFoobarString = `\u001B[31m${foobarString}\u001B[39m`; |
There was a problem hiding this comment.
Isn't this the same as foobarRed?
There was a problem hiding this comment.
Yeah, merge conflict mistake. Will fix.
| await execa('unicorns', {maxBuffer: {fd1: 0}}); | ||
| await execa('unicorns', {maxBuffer: {fd2: 0}}); | ||
| await execa('unicorns', {maxBuffer: {fd3: 0}}); | ||
| await execa('unicorns', {maxBuffer: {fd3: 0}, stdio: ['pipe', 'pipe', 'pipe', 'pipe']}); |
There was a problem hiding this comment.
Maybe we should keep the original assertion as well (without the stdio option), to make sure maxBuffer.fd3 type works, even when stdio is not set?
Same for the other changes in that same file.
| ...options, | ||
| }); | ||
|
|
||
| export const getStdioForFd3Option = (...values) => values.some(value => hasFd3Option(value)) ? fullStdio : {}; |
There was a problem hiding this comment.
If I understand correctly, this is added to tests that use some {option: {fd3: ....}} option, to make them also pass {stdio: ['pipe', 'pipe', 'pipe', 'pipe']}. However, shouldn't fd3 work even without users having to pass that stdio option?
There was a problem hiding this comment.
Not for runtime behavior. fd3 should only be valid when the child actually has fd 3, for xample via stdio: ['pipe', 'pipe', 'pipe', 'pipe']. With default stdio, fd3 should throw. The bug was that {ipc: true} added an internal pseudo-slot at index 3, so fd3 accidentally targeted IPC. This helper just makes the fd3 verbose matrix tests create a real fd3; the regression tests cover the default-stdio throw case.
| ? 'all' | ||
| : never | ||
| : `fd${FdNumber}` extends GenericOptionKeys | ||
| ? `fd${FdNumber}` |
There was a problem hiding this comment.
Do you remember the reason behind this change? Wouldn't the new code be equivalent, in principle?
I am guessing this might have to do with TypeScript doing something weird, as it sometimes does with complex logic, especially with nested conditionals. :)
There was a problem hiding this comment.
They are not equivalent. The old fallback let numeric fd lookups fall through to ipc, so buffer: {ipc: false} could make stdio[3] typed as undefined.
This keeps ipc separate: ipc only affects ipcOutput, while numeric fds only match real fdN/stdout/stderr/all. The tsd regression covers tat.
2 participants
Fixes #1239