refactor(compartment-mapper): use @endo/errors#3152
Conversation
|
boneskull
left a comment
boneskull
left a comment
There was a problem hiding this comment.
So while I don't have a major problem with landing this, I'll likely change it later once we land support for Error.code; I want all exceptions thrown (directly) out of @endo/compartment-mapper to have a proper error code.
Please let the record show I didn't ask for an implementation. 😉 Mostly I was curious about whether I needed to use details/X anywhere, and how. I should have been more specific. That said, I don't even know when to use b/bare, so I'm hoping you can enlighten me.
The reason behind the original ask was that I'm also looking into partitioning the assertions here into this module for CompartmentMapDescriptor-specific assertions and another for general, reusable assertions (e.g., assertPlainObject, assertFileUrlString), with an eye towards enabling PackageCompartmentMapDescriptor validation before returning from mapNodeModules. I saw the TODO and didn't want to propagate the same problem to a new module.
| @@ -1,5 +1,6 @@ | |||
| /* Validates a compartment map against its schema. */ | |||
|
|
|||
| import { Fail, q, b } from '@endo/errors'; | |||
There was a problem hiding this comment.
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.
| typeof value === 'string' || | ||
| Fail`${b(pathOrMessage)} in ${q(url)} must be a string; got ${q(value)}`; |
There was a problem hiding this comment.
As I noted in Slack, I am not fond of Fail generally for two reasons:
- It cannot support
Error.code - When used outside of an
assertstype guard, TS cannot infer thatFailterminates unlessthrowis used. This makes correct usage ofFailrather 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).
| * @param {string} keypath | ||
| * @param {string} url | ||
| * @returns {asserts alleged is string} | ||
| * @returns {asserts allegedLabel is string} |
| /^(?:@[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)}`; |
There was a problem hiding this comment.
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.
2 participants
Closes: #XXXX
Refs: 3fcb5b1#diff-2ce5fb201c10cb24e075359132ab825315f08bdd168d4c8e9ff464b87d3e74ffR4-R7 , #3110
Description
@boneskull reminded me of 3fcb5b1#diff-2ce5fb201c10cb24e075359132ab825315f08bdd168d4c8e9ff464b87d3e74ffR4-R7
This PR moves from the
assertstyle towards thestyle, which is more efficient in the typical non-erroneous case, and is more idiomatic for Endo. It does this at the price of introducing a new dependency from
@endo/compartment-mapperto@endo/error. If it is important for@endo/compartment-mappernot to transitively depend onses, then this PR should probably wait on #3110 or a successor that accomplishes the same goal.Security Considerations
None
Scaling Considerations
If this PR waits for #3110 or equivalent, then none. Otherwise, the transitive dependency on
sesmay be significant. OTOH,@endo/compartment-mapper's use ofses'sassertshows that it is already transitively dependent, so probably makes no difference either way.Documentation Considerations
We want to encourage the
... || Fail...style over theassertstyle, so good to get rid of violations of our own style preferences in our own code.Testing Considerations
All existing tests pass without modification, which should be good enough for this refactoring PR.
Compatibility Considerations
Some error messages that were formed with untagged template literals are now formed with
Fail. Untagged template literals don't redact, which seems to be intentional here. To preserve that intention, this PR annotations allFailinterpolations that did not already have aq(...)with ab(...).Upgrade Considerations
none