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

External secondary instances: Error conditions #202

Open
eyelidlessness opened this issue Aug 27, 2024 · 2 comments
Open

External secondary instances: Error conditions #202

eyelidlessness opened this issue Aug 27, 2024 · 2 comments
Labels
design/architecture documentation Improvements or additions to documentation Error messaging Conveying error conditions to users external-secondary-instances
Milestone

Comments

@eyelidlessness
Copy link
Member

eyelidlessness commented Aug 27, 2024

This design issue is part of broader support for external secondary instances:

This issue is focused on:

  • establishing a goal to define error conditions specific to the external secondary instance feature
  • designing the engine/client interface to document and convey such error conditions
  • anticipating nuances of error conditions as such, and other error conditions not covered by this feature but affected by this design decision

Anticipated error conditions

The error conditions I expect we'll need to handle fall into three buckets:

  • I/O failures (such as poor/intermittent connectivity, verifiable absence of referenced resources)
  • Resources available but malformed
  • Unsupported resource data types

I don't intend to detail all of the specific cases yet, but I think we should be prepared to define a specific set of expected error conditions as a prerequisite for implementation. This way we can plan and design accordingly for the various downstream user flow and/or messaging suitable for each case.

Worth mentioning on that note: depending on how we prioritize functionality, we may want to take special care to distinguish between unsupported data types we know about, and plan to support versus those we simply don't recognize.

Deeper consideration of "error" as a binary state

There's probably room for nuance in both buckets. We might want to also consider some partial and recoverable failure cases:

  • Should unavailability of some subset of form-referenced resources always block form load?
  • Can we detect poor/intermittent connectivity and retry requests?
  • Can we defer failure caused by unavailable resources, until they're required for progress filling/accessing specific subsets of a form?
  • Are there known recovery techniques for certain kinds of malformed data?

I would categorize these as future considerations, out of scope for now unless there's a compelling reason to prioritize otherwise.

But I mention them both because it's good to anticipate future goals, and because they're important to keep in mind as we consider the implications of the next section.

Goal: represent error conditions on par with success conditions

This is something I wish I had made more explicit when I wrote up #188, to avoid confusion on why some of the proposal is shaped the way it is. To be very clear:

  • Documentation-first: an ongoing goal of designs for @getodk/xforms-engine is to make documentation a core work product, in a way that is and remains consistent with the documented functionality.

  • Documentation of failure modes (and other non-happy path cases): a long-discussed goal—now coming into focus as we take on functionality with inherent fallibility—is to ensure that failure modes and error conditions are documented in a way that's consistent with success modes.

There are some fundamental limitations we face here, either imposed by the platform or our development stack (or both). In particular, since we rely heavily on the TypeScript type system to deliver documentation, we must accept that exceptions—and especially the throw keyword—are antithetical to these goals.

Options

Option 0: throw anyway

Maybe we decide that the explicit goals described above are misaligned, or that we have some other way of accommodating those goals besides avoiding throw as an error conveyance mechanism.

If we do this, I would want to have strong assurances that we can provide comparable documentation experience:

  • In editor, for projects/packages depending on @getodk/xforms-engine
  • In our generated API documentation
  • Consistent with actual implementation behavior, over time, verifiable by automation

Option 1: Result type

Fortunately, there's a wealth of prior art we can draw from! A Result type is an obvious candiate to represent the result of explicitly fallible operations. It has very clear practical benefits for our usage:

  • Known error conditions can be documented explicitly, just as successes are
  • Adopting a common representation can help promote patterns that ensure we continue to keep documentation and implementation in sync

One drawback is that a Result type specifically is not necessarily the best representation of an operation which may be partially successful, recoverable, etc. We should be prepared to adapt this approach as appropriate to any of the more ambiguous conditions described above and/or any others we may think of while we prepare for this functionality.

Option 2: some other non-Result value representation

It's possible we decide that a Result type specifically is not up to the task, perhaps because of the nuances of error conditions we expect to handle. This option could be either:

  • Another similar well-known algebraic data type (or composition of more than one)
  • Another custom tagged/discriminated union type of our own design

The latter could have some overlap with other changes we expect to introduce in the engine representation.

What about everything we already throw?

If we decide to go with either option 1 or option 2, there's a nagging problem: we already have quite a lot of throw statements throughout the codebase. Many of which are haphazard, without much if any discernable systematic consideration. Quite a few don't even throw instances of Error! 😰

I don't think this should preclude us from adopting either option. And I don't think it requires us to deal with all of this baggage upfront either. Choosing either option now, with some forethought, can give us a clear framework for improving on this extant problem, and the flexibility to do it as aggressively (or not) as we like.

Especially if we choose option 1, a project I've recently had my eye on would almost certainly be helpful: neverthrow. Aside from providing clear Result-specific patterns with convenient (even approaching idiomatic!) APIs, I also understand it to be pretty pragmatic in how it addresses the awkward "some errors known" situation we'll be in.

@lognaturel
Copy link
Member

Some possible considerations:

  • using the same mechanism for non-fatal warnings, I think that might be what Option 2 is about
  • mechanism for localization of error messages

@eyelidlessness
Copy link
Member Author

In our discussion yesterday, we generally decided to go with Option 1.

We didn't get too deep into details about what the Result type representation will be, and we didn't discuss some ancillary details like potentially using neverthrow. I think we'll benefit from this present flexibility. We can nail down the specific details as they come into focus in implementation.

There's a good possibility Options 1 and 2 may blur in that process. As mentioned in this comment:

  • We'll want to consider representation of non-fatal conditions. This is an area for which we'll want to explore some prior art, and consider whether/how a Result type is suitable for those cases. It's also possible that these are partly orthogonal concerns: a Result type is great for representing error conditions which are mutually exclusive from success; non-fatal conditions could reasonably be represented as part of the success value. Where this gets murky is wherever the distinction between fatal and recoverable itself gets subjective.

  • Localization is an important consideration. In a previous discussion with @lognaturel we considered the possibility of the engine producing a comprehensive set of known translatable keys. This would be beneficial not just at the interface aspects under design here, but consistently throughout aspects of the engine that produce human-readable strings. We'd likely want to also produce some sensible default translations of those keys for developers.

eyelidlessness added a commit that referenced this issue Oct 4, 2024
Where a “form load failure” is any Promise rejection produced by the engine’s `initializeForm` entrypoint. This is intentionally broad, and what I think is a relatively minimal first pass that might be useful to users (and to us).

“Relatively” minimal because I took the opportunity to lay a bit of groundwork for a few things either cued for near term work, or ripe for near term improvement.

Notable among those:

- Establish a more intentional approach to use of test fixtures in the `web-forms` package. This includes a dedicated fixtures directory (alongside those already used by `scenario`), and a narrower “test helper” (ugh) to access those. That access point also includes some additional breadcrumbs for ways we might improve fixture access in the future.

- `FormLoadFailure` begins laying a bit of groundwork for the incoming, much more comprehensive overhaul of error messaging.

- Actually tests a few different form load failure conditions. I believe these will be useful for catching potential regressions in that overhaul, as well in other future work around those specific failure modes.

UI/UX/presentation design note: I had a brief discussion with @alyblenkin about this being a minimal first step. We agreed that this seems like a reasonable first pass, and agreed there’s more to do in near-future iterations. Things which are currently lacking from a UX perspective, blocked by work with a larger scope:

- Better messaging of specific errors. We’ll be cataloguing known cases as part of #202, which will give us a much clearer picture of how to address this.
- Any kind of direct prompt for user action. Deferred for the same reasons.
- Ability to close the dialog or go back. Deferred because this would expand scope to navigation more generally, which we should look at more holistically.
eyelidlessness added a commit that referenced this issue Oct 4, 2024
Where a “form load failure” is any Promise rejection produced by the engine’s `initializeForm` entrypoint. This is intentionally broad, and what I think is a relatively minimal first pass that might be useful to users (and to us).

“Relatively” minimal because I took the opportunity to lay a bit of groundwork for a few things either cued for near term work, or ripe for near term improvement.

Notable among those:

- Establish a more intentional approach to use of test fixtures in the `web-forms` package. This includes a dedicated fixtures directory (alongside those already used by `scenario`), and a narrower “test helper” (ugh) to access those. That access point also includes some additional breadcrumbs for ways we might improve fixture access in the future.

- `FormLoadFailure` begins laying a bit of groundwork for the incoming, much more comprehensive overhaul of error messaging.

- Actually tests a few different form load failure conditions. I believe these will be useful for catching potential regressions in that overhaul, as well in other future work around those specific failure modes.

UI/UX/presentation design note: I had a brief discussion with @alyblenkin about this being a minimal first step. We agreed that this seems like a reasonable first pass, and agreed there’s more to do in near-future iterations. Things which are currently lacking from a UX perspective, blocked by work with a larger scope:

- Better messaging of specific errors. We’ll be cataloguing known cases as part of #202, which will give us a much clearer picture of how to address this.
- Any kind of direct prompt for user action. Deferred for the same reasons.
- Ability to close the dialog or go back. Deferred because this would expand scope to navigation more generally, which we should look at more holistically.
eyelidlessness added a commit that referenced this issue Oct 16, 2024
- For now, we’ll go with the name suggested in review. As discussed there, we have broader goals for error reporting, but we can address those in a non-bandaid change (i.e. supporting #202)
- Aligns the module and class name
@lognaturel lognaturel added this to the Next milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design/architecture documentation Improvements or additions to documentation Error messaging Conveying error conditions to users external-secondary-instances
Projects
Status: Todo
Development

No branches or pull requests

2 participants