suggestion: code style about async try catch

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.

async function to<T, E = Error> (promise: Promise<T>): Promise<[T, E]> {
  try {
    return [await promise, null as E]
  } catch (e) {
    return [null as T, e]
  }
}

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 😛.

async function () {
  const [resultA, errA] = await to(asyncFnA())
  if (errA) {
    throw new Error(`wrapper error: ${errA.message}`)
  }
}

Example

https://github.com/denoland/deno_std/blob/master/http/file_server.ts#L163

before

for (const info of fileInfos) {
  let fn = dirPath + "/" + info.name;
  if (info.name === "index.html" && info.isFile()) {
    // in case index.html as dir...
    return await serveFile(req, fn);
  }
  // Yuck!
  let mode = null;
  try {
    mode = (await stat(fn)).mode;
  } catch (e) {}
  listEntry.push({
    name: info.name,
    template: createDirEntryDisplay(
      info.name,
      fn.replace(currentDir, ""),
      info.isFile() ? info.len : null,
      mode,
      info.isDirectory()
    )
  });
}

after

for (const info of fileInfos) {
  let fn = dirPath + "/" + info.name;
  if (info.name === "index.html" && info.isFile()) {
    // in case index.html as dir...
    return await serveFile(req, fn);
  }
  const [statInfo, err] = await to(stat(fn));
  const mode = err ? null : statInfo.mode
  listEntry.push({
    name: info.name,
    template: createDirEntryDisplay(
      info.name,
      fn.replace(currentDir, ""),
      info.isFile() ? info.len : null,
      mode,
      info.isDirectory()
    )
  });
}

https://github.com/denoland/deno_std/blob/master/fs/empty_dir.ts#L8

before

export async function emptyDir(dir: string): Promise<void> {
  let items: Deno.FileInfo[] = [];
  try {
    items = await Deno.readDir(dir);
  } catch {
    // if not exist. then create it
    await Deno.mkdir(dir, true);
    return;
  }
  while (items.length) {
    const item = items.shift();
    if (item && item.name) {
      const fn = dir + "/" + item.name;
      Deno.remove(fn, { recursive: true });
    }
  }
}

after

export async function emptyDir(dir: string): Promise<void> {
  const [items, err] = await toDeno.readDir(dir));
  if (err) {
    // if not exist. then create it
    await Deno.mkdir(dir, true);
    return;
  }
  while (items.length) {
    const item = items.shift();
    if (item && item.name) {
      const fn = dir + "/" + item.name;
      Deno.remove(fn, { recursive: true });
    }
  }
}

https://github.com/denoland/deno_std/blob/master/fs/ensure_dir.ts#L7

before

export async function ensureDir(dir: string): Promise<void> {
  let pathExists = false;
  try {
    // if dir exists
    const stat = await Deno.stat(dir);
    pathExists = true;
    if (!stat.isDirectory()) {
      throw new Error(
        `Ensure path exists, expected 'dir', got '${getFileInfoType(stat)}'`
      );
    }
  } catch (err) {
    if (pathExists) {
      throw err;
    }
    // if dir not exists. then create it.
    await Deno.mkdir(dir, true);
  }
}

after

export async function ensureDir(dir: string): Promise<void> {
  const [stat, err] = await to(Deno.stat(dir));
  if (err) {
    // if dir not exists
    await Deno.mkdir(dir, true);
    return;
  }

  if (!stat.isDirectory()) {
    throw new Error(
      `Ensure path exists, expected 'dir', got '${getFileInfoType(stat)}'`
    );
  }
}

Bad part

don't support destructuring

// oops
const [{ inner }, err] = await to(promise)

no benchmark

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:

Error from Promise.reject
Error from the return value

Golang only require to handle Error from the return value

@axetroy

@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

@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)

@kevinkassimo

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

@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

@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 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.

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).

Navigation Menu

Repository navigation

suggestion: code style about async try catch #525

Why I suggest this code style

Example

Bad part

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

suggestion: code style about async try catch #525

Description

Why I suggest this code style

Example

Bad part

Activity

jas0ncn commented on Jun 26, 2019

axetroy commented on Jun 26, 2019

Dreamacro commented on Jun 26, 2019

bartlomieju commented on Jun 26, 2019

axetroy commented on Jun 26, 2019

Dreamacro commented on Jun 26, 2019

j-f1 commented on Jun 26, 2019

zekth commented on Jun 26, 2019

kevinkassimo commented on Jul 2, 2019

ry commented on Jul 7, 2019

Dreamacro commented on Jul 8, 2019

kevinkassimo commented on Jul 8, 2019

kitsonk commented on Jul 11, 2019

zekth commented on Jul 11, 2019

kitsonk commented on Jul 12, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions