linode
refactor: [M3-6852] - `FormattedAPIError`, a type-safe extension of `APIError`
Description 📝
Today, across the Cloud Manager, errors are guaranteed to conform to the universal APIError interface. This interface comes from the @linode/api-v4 package and includes a string-based reason field.
In some cases, we want to intercept errors from the API and inject formatting for UX purposes -- for example, a <Link> is injected into error messages that mention opening a support ticket. Currently, this is done by casting the formatted reason (which is natively of type JSX.Element) into the reason string field.
Historically, this has lead to crashes when string methods are applied to formatted error messages. And since the interceptors are global, it is impossibly to confidently determine which components may receive intercepted error messages.
This PR introduces a new universal error type, FormattedAPIError, which extends APIError and is native to the linode-manager package. This new interface introduces an additional field, formattedReason, which supports formatted JSX error messages. Components or utilities that need a string-based error message can fallback to the reason field, which is now guaranteed to actually be a string.
Changes 🔄
- New universal error type:
FormattedAPIError - Replace
APIErrorwithFormattedAPIErroracross the codebase - Update documentation
Preview 📷
No visual or other user-facing changes.
How to test 🧪
Since this PR replaces APIError with FormattedAPIError across the app, it is likely cumbersome to verify all individual changes. However, the following steps may assist while reviewing this PR:
- Inspect the new interface and how it is used in the app. Do you agree with this approach, the shape of the interface, and the ergonomics of using it?
- Verify there are no regressions or bugs introduced in the error handling of criticals forms in the app (e.g., Linode/Volume/OBJ/etc. create forms)
- Verify unit tests and E2E tests are passing
As an Author I have considered 🤔
Check all that apply
- [x] 👀 Doing a self review
- [x] ❔ Our contribution guidelines
- [ ] 🤏 Splitting feature into small PRs
- [x] ➕ Adding a changeset
- [x] 🧪 Providing/Improving test coverage
- [ ] 🔐 Removing all sensitive information from the code and PR description
- [ ] 🚩 Using a feature flag to protect the release
- [ ] 👣 Providing comprehensive reproduction steps
- [x] 📑 Providing or updating our documentation
- [ ] 🕛 Scheduling a pair reviewing session
- [ ] 📱 Providing mobile support
- [ ] ♿ Providing accessibility support
Do we want to actually prevent usage of
.reason? Both are required now which feels odd to me. Can you clarify the intent?
No, .reason is kept as a string fallback. There are a small number of cases that require a string (e.g., see PayPalChip).
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Similarly to @mjac0bs, I wonder about the type safety of the approach. I know we had briefly discussed alternatives in the past, but I am starting to think that the global intercept is flawed in general and each error override should be handled individually to guaranty type safety (not 100% sure but this is what my instinct tells me). This PR could serve as a base to identify all cases needed to be handled if so, unless we find a way to strengthen this approach. From a developer perspective, it sure seems to leave room for errors (including new error overrides we're adding). Happy to pair to look at this at any point.
@hkhalil-akamai I'm curious what your thoughts are between this approach and replacing errors at the component level now that you've done both this PR and https://github.com/linode/manager/pull/10390.
I think my favorite solution so far would be to just preform replacements at the component level where needed to keep things simple, but I understand that it wouldn't be as hands-off as this approach.
Example:
Similar to what Alban stated, I do think having as little reliance on intercepting as possible is a good thing for keeping things straightforward and un-abstracted. While it might be a pain, maybe we'll like having things be explicit instead of intercepted.. but I'm not sure. I think I tend to lean towards the most "direct" / un-abstracted solutions because I've disliked a lot of the abstractions made in Cloud Manager's past. I'm not saying this is one of those, I just want to make sure we're happy with it.
Curious to hear how you feel @hkhalil-akamai because you have all of the context and hands on experience with both approaches.
We can use
formattedReasonand fall back onreason, but whatever is returned by getErrorMap() (e.g.generalError) can be inaccurately typed asstring | undefinedwhen it could be (and, in the case of the account limit error, is) JSX -- unless we also change those types (L88, L109)... but the returned value is expected to be a string in a lot of places.
Great catch @mjac0bs (and I expect there to be more if we go this route) but I want to point out that this means the changes are doing exactly what was intended. Previously, getErrorMap was violating its contract by returning JSX in place of a string (leading to possible crashes and other negative consequences). Now, it is truly returning a string, leading to the less serious problem of not including a hyperlink, which is straightforward to solve by tweaking and refactoring the signature of getErorrMap.
@abailly-akamai I have considered the issue of type safety and I believe this is a sound approach. Our existing system deliberately breaks type safety (specifically this cast in interceptAPIError.tsx:18) shying away from a project-wide change like this one. While in some cases global intercepts may be undesirable, it is specifically through them that we can (and now do) guarantee the shape and type of errors from the API and avoid making this compromise.
@bnussman-akamai I am open to both solutions but I prefer the one in this PR for two reasons:
- It is extremely difficult to comprehensively predict which errors a particular API endpoint may return (due to the way errors bubble up in the API)
- This adds an additional burden to maintainers since they have to remember to format error messages when displaying them (as opposed to this approach, where they just need to remember to use
.formattedReason
Curious to hear everyone's thoughts and if there is enough interest, I can bring this to cafe for another team-wide discussion.
I think this does warrant another convo just so we're on the same page and understand all options. Can we schedule 30m outside of the cafe with a select group to get to the nitty gritty? (cafe tends to stay on the surface with a wider audience)
where they just need to remember to use
.formattedReason
If we can't enforce usage then perhaps this is not the strongest solution. If our primary concern is string methods crashing there's run time solutions to help with that. Anyway, would love to get the fullest picture before committing to this PR (which still has value!)
This PR is stale because it has been open 15 days with no activity. Please attend to this PR or it will be closed in 5 days