CesiumGS / CesiumGS/cesium-native

Use `Result` for methods that may fail

Open
#1,140 1 comment 1 reaction 0 assignees View on GitHub
enhancement quality
Dominant language
C++
Stars
623
Forks
277
PR merge metrics
No merged PRs in 30d

Description

Currently, when we have a method where the operation might fail in some way, we handle this pattern with one of the following approaches:
- Throw an `std::runtime_error` exception.
- Throw an `std::invalid_argument` exception (see #205).
- `reject` the current promise.
- Return an empty `std::optional`.

None of these are ideal approaches. In the first three, unless we have anticipated the exception and wrapped the call in a `try...catch` block, we give up any hope of gracefully handling the failure and just immediately crash. In the fourth approach, which is the one we use most often, we can usually recover from this issue, but we also don't return any information about what caused the error.

A few months ago, @kring added [CesiumUtility::Result](https://cesium.com/learn/cesium-native/ref-doc/structCesiumUtility_1_1Result.html). This is similar to the Rust `Result` type in that it lets us specify an operation that can return one of two results: a valid result, or a failure result (the difference from Rust's `Result` type is that our error type is always an `ErrorList`, not an arbitrary type). This is the "best of both worlds" compared to our current approaches: we can handle the error gracefully like returning `std::nullopt`, but we also can provide error information like when throwing an exception.

`Result` has gotten some use in new code that we've written, but we haven't extended it to the rest of the codebase. It's a breaking change, but I think it would be a breaking change that moves us in the right direction.

Contributor guide

Open the contributing guide

Research direction

No files, tests, or specific methods are named. Start by reading the linked CesiumUtility::Result documentation and inventorying existing failure-handling patterns across the codebase. Done would require an agreed scope for migrating APIs and their callers, plus updated tests, but this issue does not define either.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.