feat(ses): add support for "code" prop in SES-managed Errors#3130
feat(ses): add support for "code" prop in SES-managed Errors#3130boneskull wants to merge 3 commits into
Conversation
|
|
What I'd really like to see here is a endo/packages/ses/src/error/assert.js Lines 341 to 352 in e6d46c7 The workaround is to just wrap |
e6d46c7 to
0b2ff5a
Compare
| /** | ||
| * An `Error` with a `code` property. | ||
| */ | ||
| export type SesError = Error & { code?: string }; |
There was a problem hiding this comment.
Open to naming suggestions
There was a problem hiding this comment.
(We could have used module augmentation to extend the global Error interface with a code?: string prop, but that would affect all downstream consumers of ses, which would be inconsiderate)
There was a problem hiding this comment.
Under any name, I don't like introducing a new type for this purpose if possible.
What about cause and errors? Do they have this problem too? If not, can we treat code the same way? Once we support SuppressedError (hopefully soon) we will also need to support the further instance properties error and suppressed.
There was a problem hiding this comment.
They don't have the same problem, because Error.cause and AggregateError.errors are in the TS lib.
Our options are:
- Create a new type (as this PR does)
- Augment the global
Errortype with acodeprop
This is hazardous because it will affect downstream consumers; anyone pulling inseswill have a globalErrorwith acodeprop on it. If we do not publish the augmentation, types will work in a development environment, but will be broken downstream. - Fudge it with
// @ts-expect-erroreverywherecodeis referenced (gross)
I'm not sure what TS' policy is on adopting proposals (if they have one), but I think it's probably too early to ask.
That being said, code will/should/might end up in the TS lib eventually, at which point we can refactor away the custom type and/or alias it to Error.
kriskowal
left a comment
There was a problem hiding this comment.
Needs changeset and consensus on names.
c58a919 to
bf477a0
Compare
| /** | ||
| * An `Error` with a `code` property. | ||
| */ | ||
| export type SesError = Error & { code?: string }; |
There was a problem hiding this comment.
Under any name, I don't like introducing a new type for this purpose if possible.
What about cause and errors? Do they have this problem too? If not, can we treat code the same way? Once we support SuppressedError (hopefully soon) we will also need to support the further instance properties error and suppressed.
|
See also DRAFT PRs #2223 and #2429 . Like them, we need to coordinate this change with @endo/pass-style and @endo/marshal so that |
@erights What should I be looking at there? I'm unfamiliar with both of those packages and any discussions around either PR. I'm operating under the assumption that this PR only satisfies the (fairly minimal) requirements of I'd expect we'd extend the implementation and/or adopt in other packages when ready. Are you concerned about shipping this in |
bf3c506 to
f895a53
Compare
|
FYI, I found myself in need of creating errors with a In particular I'm adding an errors.ts:import { makeError as originalMakeError, X } from "@endo/errors";
export * from "@endo/errors";
type Details = NonNullable<Parameters<typeof originalMakeError>[0]>;
type GenericErrorConstructor = NonNullable<
Parameters<typeof originalMakeError>[1]
>;
type AssertMakeErrorOptions = NonNullable<
Parameters<typeof originalMakeError>[2]
> & {
code?: string;
};
type ChainedErrorMaker<T> = ((
template: TemplateStringsArray | string[],
...args: any[]
) => T) & {
code(code: string): ChainedErrorMaker<T>;
instance(ctor: GenericErrorConstructor): ChainedErrorMaker<T>;
options(options: AssertMakeErrorOptions): ChainedErrorMaker<T>;
};
const assertFailedDetails = X`Check failed`;
export const makeError = (
details: Details = assertFailedDetails,
errConstructor: GenericErrorConstructor | undefined = undefined,
options: AssertMakeErrorOptions | undefined = undefined,
): Error => {
const { code, sanitize = false, ...makeErrorOptions } = options ?? {};
const hasCode = code !== undefined;
const error = originalMakeError(details, errConstructor, {
...makeErrorOptions,
// `code` and `sanitize` are mutually exclusive options, ignore sanitize
sanitize: !hasCode && sanitize,
});
if (code !== undefined) {
Object.defineProperty(error, "code", {
value: code,
enumerable: false,
configurable: true,
writable: true,
});
}
return error;
};
export const fail = (
/** The details of what was asserted */
details: Details = assertFailedDetails,
/** An optional alternate error constructor to use */
errConstructor: GenericErrorConstructor | undefined = undefined,
options: AssertMakeErrorOptions | undefined = undefined,
): never => {
throw makeError(details, errConstructor, options);
};
const makeChainedHelper = <T>(
fn: (...args: Parameters<typeof makeError>) => T,
) => {
const name = fn.name && fn.name.charAt(0).toUpperCase() + fn.name.slice(1);
const maker = (
options: AssertMakeErrorOptions = {},
constructor?: GenericErrorConstructor | undefined,
): ChainedErrorMaker<T> => {
// XXX: Consider defaulting to carrying over the code of any error passed in values
// with the caveat that such value may be classified (quoted or bare)
const Fail: ChainedErrorMaker<T> = (template, ...values) =>
fn(X(template, ...values), constructor, options);
Fail.code = (code: string) => maker({ ...options, code }, constructor);
Fail.instance = (ctor: GenericErrorConstructor) => maker(options, ctor);
Fail.options = (opts: AssertMakeErrorOptions) =>
maker({ ...options, ...opts }, constructor);
Object.defineProperty(Fail, "name", { value: name });
return Fail;
};
return maker();
};
export const Fail = makeChainedHelper(fail);
export const XError = makeChainedHelper(makeError); |
@mhofman Wait does this mean I can rewrite |
f895a53 to
725213b
Compare
📚 Pull Request Stack
Managed by gh-stack |
62ebce3 to
347e386
Compare
75b5a7f to
00d996f
Compare
00d996f to
6b968ca
Compare
8df7a6a to
c3c9392
Compare
3d9cda2 to
703e928
Compare
5efc309 to
e8f139c
Compare
547293a to
9b2f8f9
Compare
9b2f8f9 to
97810ee
Compare
97810ee to
cf1c4a2
Compare
- Adds a new `string` option to `AssertMakeErrorOptions`, `code`
- If a `string`, will be an non-enumerable property of the `Error` instance returned by `makeError`.
- To avoid mutating global types, a new type `SesError` is introduced which is simply `Error & {code?: string}`.
Note that `code` is expected to be a `string` here, but only by convention; the language will not enforce this (as per the below proposal).
Ref: https://github.com/tc39-transfer/proposal-error-code-property
has nothing to do with fish
cf1c4a2 to
1ec13b7
Compare
stringoption toAssertMakeErrorOptions,codestring, will be an non-enumerable property of theErrorinstance returned bymakeError.SesErroris introduced which is simplyError & {code?: string}.Note that
codeis expected to be astringhere, but only by convention; the language will not enforce this (as per the below proposal).Ref: https://github.com/tc39-transfer/proposal-error-code-property