[RFC] Communicating failures in our APIs

[MitjaBezensek]

Problem

Our API currently does not do a good job communicating failures to the consumers. This can lead to using the API in an overly optimistic way, thinking that the API call succeeded.

The only way to prevent these kinds of issues at the moment is to do guard checks before / after calling the API, which is problematic since the are not really discoverable. You need to dig into the source code to see the ways in which API calls can fail.

Another problem is that the before checks are also not enough due to the multiplayer nature of the library. The state can change between the time you do the check and the time you perform the action.

For an example see the issue we currently have with creating shapes when max shapes has already been reached. The consumers of createShapes have no way of knowing that this problem occurred, since the createShapes method returns the editor instance in any case. This caused issues for us, even though we are a lot more familiar with our APIs, but it is probably worse for somebody that is not as familiar with the library.

Options

Do nothing

We can decide to avoid addressing this problem and just continue swallowing most of the errors. Interestingly we do throw errors in a couple of places.

createShapes<T extends TLUnknownShape>(
shapes: OptionalKeys<TLShapePartial<T>, 'id'>[]
): this

Throwing errors

API would throw errors as they are encountered. We could define different error types so that consumers get some additional information about what went wrong.

/**
 * 
 * ....
 *
 * @throws {@link MaxShapesReachedError}
 * Thrown if the maximum number of shapes per page was reached.
 * @throws {@link SomeOtherError}
 * Thrown if the bad stuff happened.
 *
 * @public
 */
createShapes<T extends TLUnknownShape>(
shapes: OptionalKeys<TLShapePartial<T>, 'id'>[]
): this 

Pros:

• This would preserve chaining.
• People are familiar with error handling.

Cons:

• Using the API would mean having to try catch all the calls that can throw if you really want to use it the way it’s intended.
• TypeScript doesn’t have support for declaring the types of thrown errors, so the DX is not the best. Can maybe use docs to specify what methods throw. Not sure if we can enforce this though?
• Throwing errors for expected failures (like the max shapes) does not feel right.
• Still not fully discoverable - code editors would not complain if you don’t account for possible errors.
• Requires quite a bit of work for the end users.

Returning errors

Similar to throwing errors, but we instead return them. This helps with the DX as we can more easily expose the types of errors that can occur in the return types of the methods.

createShapes<T extends TLUnknownShape>(
shapes: OptionalKeys<TLShapePartial<T>, 'id'>[]
): this | MaxShapesReachedError | SomeOtherError | ...

Pro:

• Different failures are more easily discoverable.

Cons:

• We’d loose chaining. Could maybe bring it back with some syntax sugar (see chain in Tiptap).
• This approach does not seem to be used that much, so it might not be something people are used to.
• To me this feels like a worse version of Result object.

Returning statuses

We could only return statuses like a true / false. Could also be a list ids of the created shapes or null / undefined if something went wrong,…

createShapes<T extends TLUnknownShape>(
shapes: OptionalKeys<TLShapePartial<T>, 'id'>[]
): boolean

Pros:

• Not as big of a change as others and also something people are used to from other APIs.
• Not as discoverable / enforceable as some other options.

Cons:

• We’d loose chaining. Could maybe bring it back with some syntax sugar (see chain in Tiptap).
• Consumers still don’t know what went wrong.

Result object

Using a Result type (similar to what we already use internally in some places). I did a quick exploration of this for createShape and createShapes methods here. I really like this pattern. If this was just for internal purposes I'd definitely go with this, but might feel a bit much to use it for the public facing API?

createShapes<T extends TLUnknownShape>(
shapes: OptionalKeys<TLShapePartial<T>, 'id'>[]
): EditorResult<void, CreateShapeError>

Pros:

• I really like this pattern.
• Not as invasive as some other options since it would not change the flow of the program (users can choose to avoid the results, which is similar to what happens now).

Cons:

• We’d loose chaining. Could maybe bring it back with some syntax sugar (see chain in Tiptap).
• It’s opinionated and might be too much to pass on to users.

[linear]

TLD-2474 [RFC] Communicating failures in our APIs

[SomeHats]

a weird idea i've not put much thought into: Editor.result

How it would work

class Editor<ResultType = unknown> {
    // Editor has a `result` property. by default it's type is `unknown` and it's set to undefined
    result: ResultType = undefined

    // we have utilities that override this property, and return `this` with the type overridden
    protected returnOk<T>(value: T): Editor<OkResult<T>> {
        (this as any).result = Result.ok(value)
        return this as Editor<OkResult<T>>
    }
    protected returnErr<E>(error: E): Editor<ErrResult<E>> {
        (this as any).result = Result.err(error)
        return this as Editor<ErrResult<E>>
    }

    // Instead of just `return this`, operations that can fail use the helpers when returning.
    // the inferred return type here is `Editor<Result<Shape[], string>>`
    createShapes() {
        if (tooManyShapes) {
            return this.returnErr('too many shapes')
        }
        // ...
        return this.returnOk(createdShapes)
    }
}

Usage

// if i don't care about results:
editor.createShapes().updateShapes().whatever

// if i do care about results:
const result = editor.createShapes().result

if (result.ok) {
    console.log('created shapes:', result.value)
}

Pros

• We keep chaining
• Users can still choose to avoid the results (less impactful that it's opinionated)
• We can still return type-safe (ish) results

Cons

• Feels weird (?)
• Some type footguns possible:

const foo = editor.createShapes()
const bar = editor.updateShapes()

if (foo.result.ok) {
    // oh no, this is actually the `Result` from `updateShapes`
}

[MitjaBezensek]

a weird idea i've not put much thought into: Editor.result

Yeah, it also crossed my mind 😄 It reminds me of working with device statuses in low level programming.

[mimecuvalo]

Here's yet another idea to mull over. For Paper we did a lot of this:

editor.execute(() => {
  ...lots of operations that manipulate the doc
})

this would create a 'call stack', as we called it (really, a transaction), that would basically handle the operations and rollback if an error was thrown while executing.

We kind of already do this with tldraw in the form of editor.batch, right? editor.batch does have a try/catch in there and that's what I would expect from an API library. I want a safe way to make some changes consistently to the canvas and I really want the library to handle any errors (the explosion should be contained within the library) and then report back to me if the operation was successful or not.

So, all this is to say, perhaps the communication of failures is maybe a mix of what you're suggesting @MitjaBezensek. We could have lower/mid-level functions throw errors. But we might communicate to consumers of the SDK to start using "safe" wrappers like batch that would give back a result object that would be help contain any messes and make things rollback nicely, etc.

Definitely worth a meeting to discuss at length!

a weird idea i've not put much thought into: Editor.result

and yeah, for this idea - another worry would be in multiplayer where .result might be the last result of another person, not necessarily something I changed. 🙃

[MitjaBezensek]

Here's yet another idea to mull over. For Paper we did a lot of this:

editor.execute(() => {
  ...lots of operations that manipulate the doc
})

this would create a 'call stack', as we called it (really, a transaction), that would basically handle the operations and rollback if an error was thrown while executing.

We kind of already do this with tldraw in the form of editor.batch, right? editor.batch does have a try/catch in there and that's what I would expect from an API library. I want a safe way to make some changes consistently to the canvas and I really want the library to handle any errors (the explosion should be contained within the library) and then report back to me if the operation was successful or not.

So, all this is to say, perhaps the communication of failures is maybe a mix of what you're suggesting @MitjaBezensek. We could have lower/mid-level functions throw errors. But we might communicate to consumers of the SDK to start using "safe" wrappers like batch that would give back a result object that would be help contain any messes and make things rollback nicely, etc.

I'd like to limit this RFC just to the low / mid level operations. Users won't just switch to higher level things, you might need more granularity in many cases so people will continue using them, and we'll also need a solution for the low / mid level if we want to build the higher level things (you only know about the issues where they happen). Happy to discuss the higher level things as well, just so we don't move in a direction that would be counterproductive in the future.

and yeah, for this idea - another worry would be in multiplayer where .result might be the last result of another person, not necessarily something I changed. 🙃

Remote changes don't go through these APIs, so I think it would probably be fine 🤷‍♂️ 😄