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

discussion: error classes #4067

Closed
ry opened this issue Feb 21, 2020 · 8 comments · Fixed by #4093
Closed

discussion: error classes #4067

ry opened this issue Feb 21, 2020 · 8 comments · Fixed by #4093

Comments

@ry
Copy link
Member

ry commented Feb 21, 2020

We recently changed the mechanism for exception handling (see #3936 )

Deno ops return an error object with a code which can be accessed via "err.kind"

  } catch (e) {
    assert(e.kind == Deno.ErrorKind.PermissionDenied);

That is still the case but now we've introduced classes for each of the Deno error types, so that exceptions can be checked using instanceof

  } catch (err) {
    assert(err instanceof Deno.Err.PermissionDenied);

My question is if we should use Deno.err.PermissionDenied? or Deno.Err.PermissionDenied ? Or Deno.Errors.PermissionDenied ? or Deno.errors.PermissionDenied?

Personally I'm in favor of Deno.err.PermissionDenied because it's the least amount of typing.

@nayeemrmn
Copy link
Collaborator

Lowercase like Deno.symbols. It's not an enum in a JS sense so no need to use the enum convention.

I prefer Deno.errors... it looks a lot better. Having Deno.symbols and Deno.err is weird, it's easier to be consistent on the non-abbreviating side.

@evenstensberg
Copy link

evenstensberg commented Feb 22, 2020

In another context: Trying to import a CJS module using createRequire throws

if (e instanceof Deno.Err.PermissionDenied)

hard to debug.

Source:

import { createRequire } from "https://deno.land/std/node/module.ts";

const selfPath = window.unescape(import.meta.url.substring(7));
const require_ = createRequire(selfPath);

Edit:

Any tips on how to debug that?

@ecyrbe
Copy link
Contributor

ecyrbe commented Feb 22, 2020

I also like Deno.errors.xxxxx better. I don't mind extra typing for clarity and consistancy.

@kevinkassimo
Copy link
Contributor

@evenstensberg You should pin your dependency, e.g. import { createRequire } from "https://deno.land/[email protected]/node/module.ts";. Without explicit version number, you'll be using the master branch, which is unstable (since it changes along with the lastest Deno source itself, which might use features that have not yet been released).

@kitsonk
Copy link
Contributor

kitsonk commented Feb 23, 2020

At first I thought Deno.Errors because namespace tend to be capitalised, but then I remembered Symbol.wellKnown contains a load of symbols, so Deno.errrors containing a bunch of error classes seems fine. I agree Err or err is way too terse.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Feb 23, 2020

Actually, would it be so bad to make them all top-level e.g. Deno.PermissionDeniedError, Deno.NotFoundError? It's closer to the web and JS built-ins.

While it looks worse, it feels better to type somehow... you're trying to differentiate between error kinds, having to write errors. before you get to the differentiating part of the error you're considering is frustrating.

That's what's nice about the Deno namespace being generally flat.

@ry
Copy link
Member Author

ry commented Feb 24, 2020

Or we could use Deno.PermissionDenied. That's the shortest yet...

assert(e instanceof Deno.errors.PermissionDenied);

I really hate such long APIs... but maybe it's okay.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 24, 2020

Only problem is the name of the class would be PermissionDenied and lose the error. I don't know if that is a problem, but default logging would be:

error: Uncaught PermissionDenied: msg

Instead of:

error: Uncaught PermissionDeniedError: msg

Which is more aligned to other JavaScript errors.

@ry ry closed this as completed in #4093 Feb 24, 2020
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

Successfully merging a pull request may close this issue.

6 participants