chore: bump minimum required Deno version to 1.46.0 and cleanup #332
Conversation
Our current minimum supported Deno CLI version is 1.41.1, which has builtin support for `isTerminal` method on various i/o streams. So we no longer need to have our custom version.
|
Blocked by denoland/deno_emit#167 as deployctl relies on |
magurotuna
changed the title
magurotuna
changed the title
| // deno-lint-ignore no-explicit-any | ||
| export function isTerminal(stream: any) { | ||
| if ( | ||
| semverGreaterThanOrEquals( | ||
| semverParse(Deno.version.deno), | ||
| semverParse("1.40.0"), | ||
| ) | ||
| ) { | ||
| return stream.isTerminal(); | ||
| } else { | ||
| // deno-lint-ignore no-deprecated-deno-api | ||
| return Deno.isatty(stream.rid); | ||
| } | ||
| } |
There was a problem hiding this comment.
For reviewers: isTerminal method on various stream types is available on 1.40.0+ and our minimum required Deno version is now 1.46.0. We don't have to polyfill anymore.
src/error.ts
Outdated
| function stringifyError(err: Error): string { | ||
| const cause = err.cause === undefined | ||
| ? "" | ||
| : `Caused by ${stringify(err.cause)}`; |
There was a problem hiding this comment.
Maybe add the new line in here, this way we don't get an empty line if there's no cause.
Having said that, being a CLI tool, deployctl is sensible to changes in the formatting of the error messages. I'm a bit concerned that adding this newline will break the formatting here and there inadvertently. Maybe we could use parenthesis (${cause}) instead of new line \n${cause}?
There was a problem hiding this comment.
Can you elaborate a bit more on why CLI tools are sensible to the change of error message formats?
Maybe we could use parenthesis (${cause}) instead of new line \n${cause}?
Parenthesis sounds good, but the next problem is that cause itself might contain newline characters in it. I'll try to replace them (if any) with white spaces.
There was a problem hiding this comment.
There are places where the error is not fatal, and should be shown to the user as part of the regular output. If the error has multiple lines it breaks the output layout. One example is #332 (comment)
There was a problem hiding this comment.
That makes lots of sense. I'll create another function to stringify errors in a simple, one-line format rather than full verbose ones so we can use it in non-fatal places.
There was a problem hiding this comment.
Now stringify function gets verbose option which defaults to false. If an error is printed when the deployctl program keeps running, simple message is displayed. Otherwise i.e. when the program exits right after the error message is printed, verbose message is generated.
| @@ -76,7 +77,7 @@ async function provision(): Promise<string> { | |||
| wait("").start().warn( | |||
| "Unexpected error while trying to open the authorization URL in your default browser. Please report it at https://github.com/denoland/deployctl/issues/new.", | |||
| ); | |||
| wait({ text: "", indent: 3 }).start().fail(error.toString()); | |||
| wait({ text: "", indent: 3 }).start().fail(stringifyError(error)); | |||
There was a problem hiding this comment.
Can you check if this change breaks the output format? IIRC it was sensible to multiline error messages.
There was a problem hiding this comment.
If an error is formatted in a multi-line format, it is printed like this:
This error instance is created as follows:
const e1 = new Error("This is\na multiline\nerror");
const e2 = new Error("e2", { cause: e1 });
throw e2;I am not sure if I understand your concern, but does this look concerning to you?
There was a problem hiding this comment.
It's purposedly indentated and kept in a single line because it is not a hard error, the program continues. If the error breaks this identation then it breaks the output layout.
There was a problem hiding this comment.
Now stringifyError(error) returns a simple one-line error message while stringifyError(error, { verbose: true }) generates a full detailed output.
|
Also make sure to update the README with the new minimum version |
…no process)" This reverts commit 1f0f76985a5fb05d9744a9ff11d4004dd515eb67.
This commit bumps the minimum required Deno version to 1.46.0 and cleanups the code mainly to deal with deprecated Deno APIs.
Fixes #328