fix(compartment-mapper): fix ReadPowers-related types #2188
Conversation
This change: 1. Creates a minimal interface for the `fs`, `url`, and `crypto` objects as passed into `makeReadPowers()`. This makes it easier to duck-type the objects. 2. Fixes the invalid type of `MaybeReadPowers`; properties (defined thru `@property`) are ignored in a `@typedef` of that `@typedef` does not extend `object`/`Object`. 3. Added necessary type assertion in `powers.js` 4. Adds return type to `makeReadPowersSloppy()` # Conflicts: # packages/compartment-mapper/src/types.js
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @boneskull and the rest of your teammates on |
| * @param {import('./types.js').FsAPI} args.fs | ||
| * @param {import('./types.js').UrlAPI} [args.url] | ||
| * @param {import('./types.js').CryptoAPI} [args.crypto] |
There was a problem hiding this comment.
I’m a tiny bit worried that, if we need more powers from these modules, reaching them going forward will constitute a breaking change. Is it possible to address your concerns without narrowing these types?
There was a problem hiding this comment.
I don't see it as possible to do without breaking things, no. But currently, we're asking for way more than we need to.
I'd even be amenable to ditching these objects entirely, and asking for the specific functions in isolation--i.e., instead of discrete fs, url and crypto objects, we'd ask for a bucket of functions matching what we need.
There was a problem hiding this comment.
I’m with you on principle of least authority but not aligned on resilience to change. This is a tool for attenuating the Node.js builtins down to the powers the compartment mapper already needs. The powers space is very narrowly scoped and any new powers have to be optional and have backward-compatible fallbacks. The attenuator however benefits from being able to reach for more powers as needed to upgrade gracefully. Last time we needed this, it was to reach for fs.realpath in order to canonicalize. I hope the dependencies for compartment mapper are stable, but I’m not sure.
| @@ -18,7 +18,8 @@ export const unpackReadPowers = powers => { | |||
| if (typeof powers === 'function') { | |||
| read = powers; | |||
| } else { | |||
| ({ read, maybeRead, canonical } = powers); | |||
| ({ read, maybeRead, canonical } = | |||
| /** @type {import('./types.js').MaybeReadPowers} */ (powers)); | |||
There was a problem hiding this comment.
I think this is actually MaybeReadPowers | ReadPowers, but we don't know until L29
| @@ -165,8 +165,7 @@ export {}; | |||
| */ | |||
|
|
|||
| /** | |||
| * @typedef {ReadPowers | object} MaybeReadPowers | |||
| * @property {MaybeReadFn} maybeRead | |||
| * @typedef {ReadPowers & {maybeRead: MaybeReadFn}} MaybeReadPowers | |||
There was a problem hiding this comment.
The old pattern broken in general? Used to be vogue.
There was a problem hiding this comment.
I don't recall that ever working. To combine types in a @typedef, I've always needed to use intersections or something akin to type-fest's Merge.
As written, it looks like it might work (it does not), but for the wrong reasons--afaict ReadPowers | object simplifies to object.
Description
This change:
fs,url, andcryptoobjects as passed intomakeReadPowers(). This makes it easier to duck-type the objects. The drawback here is that it is not future-proof, and these types will need to be updated if we end up using additional methods on these objects.MaybeReadPowers; properties (defined thru@property) are ignored in a@typedefof that@typedefdoes not extendobject/Object.powers.jsmakeReadPowersSloppy()Upgrade Considerations
This is a type-only change.