[@types/node] SpawnSyncReturns may have null values #25731
Conversation
When spawn a program that doesn't exist
```javascript
const res = child_process.spawnSync('UnknownProgram')
res.status // null
res.signal // null
res.output // null
res.stdout // null
res.stderr // null
res.Error // Error
```
When successfully spawn a program
```javascript
const res = child_process.spawnSync('echo', ['hello'])
res.Error // null
res.signal // null
res.output // [null, Buffer('hello\n'), Buffer(0)]
```
��
|
@KSXGitHub Thank you for submitting this PR! 🔔 @Alorel @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @eps1lon @Hannes-Magnusson-CK @jkomyno @hoo29 @n-e @BrunoScheufler @islishude @ajafff @mohsen1 @a-tarasyuk - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
Although CI had failed at |
|
@KSXGitHub The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
Related pull request: DefinitelyTyped#25731
|
@Alorel please review changes I made for |
|
@KSXGitHub The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
Breaking change is expected (I had to also edit |
|
@KSXGitHub There is no semver versioning on typings... see #25677 for details. |
|
Personally, I think this PR is necessary. The very purpose purpose of TypeScript is type-safety, the old definition fails to do so by wrongly assume |
|
I'm not 100% sure here. Therefore if we would change this and you check in your code that So it's a matter of correctness vs. usability. It would be great if typescript would allow to express such type relationships to tell that |
|
I made this pull request because I encountered a very bullshit bug in my project due to the use of |
types/node/index.d.ts
Outdated
| stderr: T | null; | ||
| status: number | null; | ||
| signal: string | null; | ||
| error: Error | null; |
There was a problem hiding this comment.
can this be converted to a discriminated union with error as the discriminator?
There was a problem hiding this comment.
Maybe, I'm 100% sure how exactly spawnSync works. But I am sure though, when error is not null, the other properties are. I'm not so sure about signal.
There was a problem hiding this comment.
I also hope that no fools create an interface that extends this one. 😄
|
@ajafff So, about updating type ValueCarrier<X> =
ValueCarrier.WithoutError<X> | ValueCarrier.WithError
namespace ValueCarrier {
export interface WithError {
error: Error
value: null
}
export interface WithoutError<Value> {
error: null
value: Value
}
}
declare function getVal<X>(): ValueCarrier<X>
/* MAIN */
const carrier = getVal<string>()
if (carrier.error === null) {
const error: null = carrier.error
const value: string = carrier.value // Error here...
} else {
const error: Error = carrier.error // And here
const value: null = carrier.value
}Perhaps I should restore the old version of test. |
|
Nope. |
|
@ajafff Since |
|
@KSXGitHub I agree. If TypeScript doesn't handle it correctly, using tagged unions is unnecessary and should be reverted. |
|
@ajafff Since reverting it makes practically no differences, I would like to keep it in hope that TypeScript will fix it soon. |
|
By the way, when I read |
I don't know why it's disabled. But I do know that this causes the tagged union to not work as expected. |
|
Because node type definitions have been created before typescript 2.0 (and therefore |
|
So just to summarize for myself, this is a breaking change and currently TypeScript doesn't use |
|
@DanielRosenwasser Yes. |
Node.js v10.2.0 just released. |
|
@KSXGitHub The release cycle of NodeJS and of @types/node is not related. NodeJS exposes/documents javascript APIs and doesn't care about typescript. |
|
I've opened microsoft/TypeScript#24479. I feel slightly uneasy merging this in without that issue fixed. |
|
@KSXGitHub The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@KSXGitHub To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you! |
When spawn a program that doesn't exist
When successfully spawn a program
Please fill in this template.
npm test.)npm run lint package- 8000 name(ortscif notslint.jsonis present).Select one of these and delete the others:
If adding a new definition:
.d.tsfiles generated via--declarationdts-gen --dt, not by basing it on an existing project.tslint.jsonshould be present, andtsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.If removing a declaration:
notNeededPackages.json.