Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggestion: code style about async try catch #525

Closed
Dreamacro opened this issue Jun 26, 2019 · 15 comments
Closed

suggestion: code style about async try catch #525

Dreamacro opened this issue Jun 26, 2019 · 15 comments

Comments

@Dreamacro
Copy link
Contributor

Dreamacro commented Jun 26, 2019

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
@jas0ncn
Copy link

jas0ncn commented Jun 26, 2019

Awesome! The return type of to maybe Promise<[T, null] | [null, E]>, which more semantic.

@axetroy
Copy link
Contributor

axetroy commented Jun 26, 2019

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:

  1. Error from Promise.reject
  2. Error from the return value

Golang only require to handle Error from the return value

@Dreamacro
Copy link
Contributor Author

@axetroy It doesn't mean exposing to as an std function, but it really improves std library code readability

@bartlomieju
Copy link
Member

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.

@axetroy
Copy link
Contributor

axetroy commented Jun 26, 2019

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.

@Dreamacro
Copy link
Contributor Author

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

@j-f1
Copy link

j-f1 commented Jun 26, 2019

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)}'`
    );
  }
}

@zekth
Copy link
Contributor

zekth commented Jun 26, 2019

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.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jul 2, 2019

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)

@ry
Copy link
Member

ry commented Jul 7, 2019

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 closed this as completed Jul 7, 2019
@Dreamacro
Copy link
Contributor Author

@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

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jul 8, 2019

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

@kitsonk
Copy link
Contributor

kitsonk commented Jul 11, 2019

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.

@zekth
Copy link
Contributor

zekth commented Jul 11, 2019

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.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 12, 2019

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

inverted-capital pushed a commit to dreamcatcher-tech/napps that referenced this issue Oct 24, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This PR applies various fixes, cleanups and improvements to Stripe
logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants