fix: add support for typescript 2.6 #74
Conversation
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
=========================================
+ Coverage 99.6% 99.6% +<.01%
=========================================
Files 8 8
Lines 251 254 +3
Branches 10 10
=========================================
+ Hits 250 253 +3
Misses 1 1
Continue to review full report at Codecov.
|
package.json
Outdated
| @@ -66,7 +66,7 @@ | |||
| "nyc": "^11.2.1", | |||
| "source-map-support": "^0.5.0", | |||
| "tmp": "0.0.31", | |||
| "typescript": "~2.4.1" | |||
| "typescript": "^2.6.1" | |||
There was a problem hiding this comment.
Do we want to go with the ~ here?
There was a problem hiding this comment.
Thanks for catching that!
| @@ -22,18 +22,33 @@ const simpleExecp = pify(cp.exec); | |||
| const renamep = pify(fs.rename); | |||
| const ncpp = pify(ncp.ncp); | |||
|
|
|||
| // TODO: improve the typedefinitions in @types/node. Right now they specify | |||
| // the return type to be Error. | |||
| interface ExecError extends Error { | |||
There was a problem hiding this comment.
This is non-blocking PR commentary :) I see the pattern of attaching a code property to Error objects all over our repositories. It makes working with TypeScript and errors kind of ugly. Two thoughts on how to address:
-
Should we maybe consider using Error.prototype.name instead?
-
Should we consider chatting with tc39 folks about adding a general purpose
codefield to the Error object?
There was a problem hiding this comment.
@types/node defines NodeJS.ErrnoException, which extends Error and has errno, code, path, syscall, and stack. Is that what we want?
There was a problem hiding this comment.
@jinwoo No. ErrnoException has a string code. child_process.exec API returns an Error with a number code property. @types/node doesn't have an error type for the exec at the moment.
There was a problem hiding this comment.
Should we maybe consider using Error.prototype.name instead?
This is not up to us. The exec API in Node.js core returns an Error with a particular structure.
Should we consider chatting with tc39 folks about adding a general purpose code field to the Error object?
In Node.js core, James Snell is championing an effort to have a code property added to all errors returned by core. I think it would be worth chatting with him to the next steps beyond that.
|
TBR= @kjin |
| } | ||
|
|
||
| function isExecError(err: Error|ExecError): err is ExecError { | ||
| return (<ExecError>err).code !== undefined; |
There was a problem hiding this comment.
I think we prefer err as ExecError?
There was a problem hiding this comment.
Also, isExecError could have checked that err is undefined.
| const execp = | ||
| (command: string, execOptions?: cp.ExecOptions): Promise<ExecResult> => { | ||
| return new Promise((resolve) => { | ||
| cp.exec( | ||
| command, execOptions || {}, | ||
| (err: Error&{code: number}, stdout, stderr) => { | ||
| resolve({exitCode: err ? err.code : 0, stdout, stderr}); | ||
| (err: Error|ExecError|null, stdout, stderr) => { |
There was a problem hiding this comment.
Should this be ExecError|null? This looks like the same as googleapis/gcp-metadata#16
If so, I think isExecError might not be needed.
No description provided.