Closed Dreamacro closed 5 years ago
Awesome! The return type of to
maybe Promise<[T, null] | [null, E]>
, which more semantic.
it's a bit too non-standard for me.
The standard library should avoid some magic-hacks.
Not the same as Golang
, this requires handle with two types of errors:
Promise.reject
Golang only require to handle Error from the return value
@axetroy It doesn't mean exposing to
as an std function, but it really improves std library code readability
Not so long ago we had similar syntax to this example:
const [items, err] = await toDeno.readDir(dir));
It was deemed non-idiomatic TypeScript and I agree that try/catch
is better.
Check #472 for more details.
In fact, this style of interface return is not impossible.
But limited to some specific interfaces
const [byte, error] = await copy(writer, reader);
// byte: record how many bytes have been copied
// error: Whether an error has occurred, including promise reject.
In this scenario, you need to accurately get the number of bytes of copy, even if the entire copy fails.
Unfortunately, Deno.copy()
does not have this implementation.
@bartlomieju
In my opinion, no every try/catch
need this helper function.
It is used in somewhere "make sense" like ensure_dir.ts
(When I refactored ensureDir
, I found that Its logic is a bit complicated.)
You could always use the native promise methods:
const mode = await stat(fn).catch(() => null);
const items = await Deno.readDir(dir).catch(() => Deno.mkdir(dir, true).then(() => []));
export async function ensureDir(dir: string): Promise<void> {
let pathExists = false;
const stat = await Deno.stat(dir).catch(() => await Deno.mkdir(dir, true));
if (stat && !stat.isDirectory()) {
throw new Error(
`Ensure path exists, expected 'dir', got '${getFileInfoType(stat)}'`
);
}
}
Really looks like Haskell's Either
: https://www.schoolofhaskell.com/school/starting-with-haskell/basics-of-haskell/10_Error_Handling
This may be used in other modules but not standard.
I do think we still want to stick with idiomatic JS try-catch, but I feel this wrapping to
function (we might want to rename it to something else, maybe like asResult
) would be a good util to include in https://github.com/denoland/deno_std/blob/master/util/async.ts
Also maybe we could implement Option
or Result
types (similar to the one in Rust) and wrapping helpers as a module here that people could use if they prefer that way (merely as an option), like this one: https://gist.github.com/s-panferov/575da5a7131c285c0539
(I do prefer it over Go's tuple-based results)
I think @kevinkassimo put it well. For std it's best to s itick with idiomatic JS, but it would be interesting to see experiments with other methods.
@ry The reason that prompted me to open this issue is to see the following code in std
let mode = null;
try {
mode = (await stat(fn)).mode;
} catch (e) {}
I think this code should be avoided in std
@Dreamacro Yeah I agree this looks strange. At lease I should have just do try { ... } catch { ... }
without (e)
part.
The idea is that for this single case the error is not important and we just want to leave it as null if an error occurs. But in many other cases you do want to handle the error with the try-catch block, which is idiomatic JS.
If we were to really implement something to make this easier here, I would suggest some helper like
const statInfo = await maybe(stat(fn)); // yields FileInfo | null
const mode = statInfo ? statInfo.mode : null;
// I do even want to use optional chaining once it is approved by TC39,
// so that I could do just `const mode = (await maybe(stat(fn)))?.mode;`
We could implement this in utils/async.ts
for our own use and provided as an option to the user. But as I said, there are more cases where try-catch is the right way to go in JS, or providing catch
handlers like @j-f1 mentioned above.
(As an exaggerated example, global errno
in C might be cleaner for some cases, but we don't do constructs like this in JS since it is going against the expected language usages. Similar reasons why most C++ and Rust people don't return tuples for error handing)
If you believe these helpers are very helpful, would be great if you could contribute and submit a PR to add these to utils/async.ts
, and potentially in the future persuade more JS developers to try such way of handling such that one day it might be idiomatic enough
(I am currently bound by some legal hassles so unfortunately I cannot do it myself)
Yeah, we should omit the (e)
if not used in the catch
block. That is allowed syntax in the current ECMAScript and is support both by the version of V8 and TypeScript we are using.
Yeah, we should omit the
(e)
if not used in thecatch
block. That is allowed syntax in the current ECMAScript and is support both by the version of V8 and TypeScript we are using.
ESlint would yell if not used.
ESlint would yell if not used.
No it doesn't. The binding was not optional until recently, so it will just allow catch (e)
because that was the only valid way to right it. ESLint is still debating the rule. The optional binding for catch
statements is only part of ES2019 published in June (though there was a lot of adoption earlier than that, just not by eslint).
Why I suggest this code style
I have a quick look at
deno_std
and find out that the "try-catch" code style could be improved. I use a little trick about "try-catch" to improve code structure in the real world.it's helpful when we want to throw a wrapper error and reduce indent deep to make code clear. btw, it seems like golang error handle 😛.
Example
https://github.com/denoland/deno_std/blob/master/http/file_server.ts#L163
before
after
https://github.com/denoland/deno_std/blob/master/fs/empty_dir.ts#L8
before
after
https://github.com/denoland/deno_std/blob/master/fs/ensure_dir.ts#L7
before
after
Bad part