Skip to content

refactor(compartment-mapper): use @endo/errors #3152

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.

Sign up for GitHub

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

Open
erights wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −35
Open

refactor(compartment-mapper): use @endo/errors #3152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/compartment-mapper/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
},
"dependencies": {
"@endo/cjs-module-analyzer": "workspace:^",
"@endo/errors": "workspace:^",
"@endo/module-source": "workspace:^",
"@endo/trampoline": "workspace:^",
"@endo/zip": "workspace:^",
Expand Down
54 changes: 19 additions & 35 deletions packages/compartment-mapper/src/compartment-map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* Validates a compartment map against its schema. */

import { Fail, q, b } from '@endo/errors';
Copy link
Copy Markdown
Member

@boneskull boneskull Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we're pulling in @endo/errors if all these symbols are already available in globalThis. I'm missing some context.

import {
assertPackagePolicy,
ATTENUATORS_COMPARTMENT,
Expand Down Expand Up @@ -31,16 +32,11 @@ import {
* DigestedCompartmentDescriptor} from './types.js'
*/

// TODO convert to the new `||` assert style.
// Deferred because this file pervasively uses simple template strings rather than
// template strings tagged with `assert.details` (aka `X`), and uses
// this definition of `q` rather than `assert.quote`
const q = JSON.stringify;
const { keys, entries } = Object;
const { isArray } = Array;

/** @type {(a: string, b: string) => number} */
// eslint-disable-next-line no-nested-ternary
// eslint-disable-next-line no-nested-ternary, no-shadow
export const stringCompare = (a, b) => (a === b ? 0 : a < b ? -1 : 1);

/**
Expand Down Expand Up @@ -83,12 +79,8 @@ function* enumerate(iterable) {
* @returns {asserts value is string}
*/
const assertString = (value, pathOrMessage, url) => {
const keypath = pathOrMessage;
assert.typeof(
value,
'string',
`${keypath} in ${q(url)} must be a string; got ${q(value)}`,
);
typeof value === 'string' ||
Fail`${b(pathOrMessage)} in ${q(url)} must be a string; got ${q(value)}`;
Comment on lines +82 to +83
Copy link
Copy Markdown
Member

@boneskull boneskull Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I noted in Slack, I am not fond of Fail generally for two reasons:

  1. It cannot support Error.code
  2. When used outside of an asserts type guard, TS cannot infer that Fail terminates unless throw is used. This makes correct usage of Fail rather tricky.

We could mitigate 1. by introducing a Fail.code(str)-like API, but that's very much a separate PR & discussion.

I have a branch where I've replaced all thrown errors (including usage of Fail, throw new Error(), & throw Error()) with a wrapper using makeError(), built on top of #3130. While this would be fine to merge as-is, I would want to replace the Fail calls (regardless of whether Fail.code() becomes a thing).

};

/**
Expand All @@ -97,7 +89,7 @@ const assertString = (value, pathOrMessage, url) => {
* @param {unknown} allegedLabel
* @param {string} keypath
* @param {string} url
* @returns {asserts alleged is string}
* @returns {asserts allegedLabel is string}
Copy link
Copy Markdown
Member

@boneskull boneskull Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

*/
const assertLabel = (allegedLabel, keypath, url) => {
assertString(allegedLabel, keypath, url);
Expand All @@ -107,12 +99,10 @@ const assertLabel = (allegedLabel, keypath, url) => {
if (allegedLabel === ENTRY_COMPARTMENT) {
return;
}
assert(
/^(?:@[a-z][a-z0-9-.]*\/)?[a-z][a-z0-9-.]*(?:>(?:@[a-z][a-z0-9-.]*\/)?[a-z][a-z0-9-.]*)*$/.test(
allegedLabel,
),
`${keypath} must be a canonical name in ${q(url)}; got ${q(allegedLabel)}`,
);
/^(?:@[a-z][a-z0-9-.]*\/)?[a-z][a-z0-9-.]*(?:>(?:@[a-z][a-z0-9-.]*\/)?[a-z][a-z0-9-.]*)*$/.test(
allegedLabel,
) ||
Fail`${b(keypath)} must be a canonical name in ${q(url)}; got ${q(allegedLabel)}`;
Copy link
Copy Markdown
Member

@boneskull boneskull Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little unclear on where we want to use b/bare vs q/quote; obviously the latter is for strings, but from what I can tell, keypath is also a string.

};

/**
Expand All @@ -123,12 +113,10 @@ const assertLabel = (allegedLabel, keypath, url) => {
*/
const assertPlainObject = (allegedObject, keypath, url) => {
const object = Object(allegedObject);
assert(
object === allegedObject &&
!isArray(object) &&
!(typeof object === 'function'),
`${keypath} must be an object; got ${q(allegedObject)} of type ${q(typeof allegedObject)} in ${q(url)}`,
);
(object === allegedObject &&
!isArray(object) &&
!(typeof object === 'function')) ||
Fail`${b(keypath)} must be an object; got ${q(allegedObject)} of type ${q(typeof allegedObject)} in ${q(url)}`;
};

/**
Expand All @@ -139,19 +127,16 @@ const assertPlainObject = (allegedObject, keypath, url) => {
* @returns {asserts value is boolean}
*/
const assertBoolean = (value, keypath, url) => {
assert.typeof(
value,
'boolean',
`${keypath} in ${q(url)} must be a boolean; got ${q(value)}`,
);
typeof value === 'boolean' ||
Fail`${b(keypath)} in ${q(url)} must be a boolean; got ${q(value)}`;
};

/**
* @param {Record<string, unknown>} object
* @param {string} message
*/
const assertEmptyObject = (object, message) => {
assert(keys(object).length === 0, message);
keys(object).length === 0 || Fail`${b(message)}`;
};

/**
Expand All @@ -161,10 +146,9 @@ const assertEmptyObject = (object, message) => {
*/
const assertConditions = (conditions, url) => {
if (conditions === undefined) return;
assert(
isArray(conditions),
`conditions must be an array; got ${conditions} in ${q(url)}`,
);
if (!isArray(conditions)) {
throw Fail`conditions must be an array; got ${b(String(conditions))} in ${q(url)}`;
}
for (const [index, value] of enumerate(conditions)) {
assertString(value, `conditions[${index}]`, url);
}
Expand Down
1 change: 1 addition & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ __metadata:
dependencies:
"@endo/cjs-module-analyzer": "workspace:^"
"@endo/env-options": "workspace:^"
"@endo/errors": "workspace:^"
"@endo/evasive-transform": "workspace:^"
"@endo/eventual-send": "workspace:^"
"@endo/init": "workspace:^"
Expand Down
Loading