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

Specify error mappings #57

Open
a-sully opened this issue Oct 3, 2022 · 3 comments
Open

Specify error mappings #57

a-sully opened this issue Oct 3, 2022 · 3 comments

Comments

@a-sully
Copy link
Collaborator

a-sully commented Oct 3, 2022

This came up in web-platform-tests/wpt@b20e834#r85726061

Ideally API errors map uniquely to DOMExceptions, which make them actionable by the site. For example, if NoModificationAllowedError always indicates that a file is locked and NotAllowedError always indicates the site needs to requestPermission()on the handle, the site can respond differently to these errors.

Currently, developers have to figure out this mapping on their own, or read the whole spec and infer this mapping. It would be nice to include a table mapping API errors to DOMExceptions that developers can use as a reference. Is there precedent for having this type of table in a spec?

That being said, it could be tricky to add a mapping that truly guarantees an error corresponds to a given API error (especially outside of the OPFS, where there's a whole host of things that could happen on the underlying OS). At the very least, here's a list of the mappings I'm aware of as a starting point...

  • NotAllowedError
    • No permission at the API level (i.e. need to requestPermission())
  • NoModificationAllowedError
    • Cannot acquire a lock to the handle
    • Handle lacks read or write permission at the OS level (for example, trying to get write permission on a read-only file). This should only be relevant outside of the OPFS
  • InvalidModificationError
    • Attempting to overwrite a file via move()
  • QuotaExceededError
    • Operation will exceed quota
  • TypeMismatchError
    • The handle type (file/dir) does not match what's on disk
  • AbortError
    • Using a not-yet-launched feature (i.e. directory move())
    • Some internal errors
  • InvalidStateError
    • Attempting to close an already-closed writable or SyncAccessHandle
    • Some internal errors

Some errors are mostly relevant outside of the OPFS:

  • AbortError (in addition to above)
    • Picker selection cancelled
    • File save/move blocked by Safe Browsing
  • InvalidStateError (in addition to above)
    • Trying to show a picker in a detached frame
  • SecurityError
    • Trying to show picker in sandboxed iframe/opaque origin/third party context/without user gesture
@annevk
Copy link
Member

annevk commented Oct 10, 2022

"AbortError" is very specific and mainly for abort signals only. cc @domenic

I'd also recommend considering that implementations might want some flexibility going forward so requiring rather unique exceptions might not be worthwhile if web developers aren't expected to branch on the exception names anyway.

@jesup
Copy link
Contributor

jesup commented Oct 13, 2022

If there's specific action an application might want to take for a specific failure mode, then tightly specifying errors would be useful. Otherwise tightly specifying errors doesn't have much benefit I can see; though specifying them avoids unintentional lock-in by developers who look for a specific (non-specced) error, and then break if it changes -- or if they use a different browser. However, we should be in alignment with how other specs do (or should) specify errors. @annevk ? @domenic ?

We throw SecurityError for some cases that violate security rules.

Instead of AbortError for non-implemented features, maybe NotSupportedError?

@domenic
Copy link
Member

domenic commented Oct 13, 2022

So you definitely need to specify the error type, and all browsers need to agree. There's no option of not specifying the error type at all.

But you don't need to make sure every error type is used perfectly and maps to distinct cases, if developers don't need to distinguish between such cases. For example, you could just always throw a TypeError (the web platform's generic error). Or, you could just lump a bunch of error conditions together under "OperationError" DOMException, or "InvalidStateError" DOMException. That's a reasonable option.

Specific error type guidance you should be consistent with, which is unfortunately not written down or consistently applied, is:

  • Use "NotAllowedError" DOMException for permissions
  • Use "SecurityError" DOMException for security checks
  • Use "NotSupportedError" DOMException for not-yet-launched features
  • Use "AbortError" DOMException for operations that were aborted, usually in the sense of the developer requesting or causing the abort, usually via an AbortSignal.
  • Use "UnknownError" DOMException for internal, implementation-specific issues.

Note that "TypeMismatchError" DOMException is deprecated.

So yeah, there are some improvements you could make here to align better with the rest of the platform.

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 15, 2022
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/tESXuOfqQMQ

Also creates a flag to disable this feature if needed, which would
reject with a NotSupportedError (as suggested here:
whatwg/fs#57)

Bug: 1114923
Change-Id: I6568652a1e3e73f69017d5349a62d65971d61301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4077526
Reviewed-by: Daseul Lee <[email protected]>
Commit-Queue: Daseul Lee <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Reviewed-by: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1083453}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants