feat(compartment-mapper): add option for custom logger to varied functions

[boneskull]

Description

I'd like @endo/compartment-mapper to a) emit more information about what it's doing and b) allow me to control how it emits that information.

In lieu of a larger complicated reporting/diagnostics system (potentially invoking the spectre of EventEmitter), I've allowed a custom logger to be passed in to a handful of functions in @endo/compartment-mapper (including both private and public APIs). Generally speaking, @endo/compartment-mapper currently doesn't log anything (except the one place that it does); I intend to add more log() calls in future PRs to aid debugging.

This change:

• adds a LogFn type which can be accepted in an options bucket (LogOptions)
• mapNodeModules and its internal functions now support this option
• search, captureFromMap, loadLocation and importLocation all support this option (but do not yet use it)
• rewrote a (the) log message in graphPackage to use a static string message and data object
• added types/node-modules.ts for types specific to node-modules.js; these should all be considered internal
• moved some typedefs from node-modules.js into types/node-modules.ts
• created a PackageDescriptor type, which is a rough approximation of a package.json and leveraged it throughout functions involved in reading one
• cleaned up weird type in policy.js
• refactored signatures of some internal functions to use options buckets

Security Considerations

None that I'm aware of, but we should be careful about what and how we log in the future. @kriskowal suggested that log messages should be static, and all dynamic context should be provided to the logger as data.

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

Tests may provide null loggers to functions which accept LogFns.

Compatibility Considerations

This will break anybody parsing the single extant log message emitted from @endo/compartment-mapper, since the message is now different. This is unlikely to affect anyone—but if it does, I will personally apologize to them.

Upgrade Considerations

Will need to update NEWS.md.

[boneskull]

I chose to add the log option to the functions in question because they are the functions I am consuming in @lavamoat/node; there are likely to be more, but mapNodeModules was my top concern.

[boneskull]

Question: Should console.warn() be the default logger?

I'd prefer the default be a no-op (() => {}) but did not want to break it too badly.

But if we are to use console.something(), and we are not differentiating between "error levels", then console.error() seems most appropriate given its ubiquitous use as a log to stderr without a presumed severity.

[kriskowal]

Question: Should console.warn() be the default logger?

I'd prefer the default be a no-op (() => {}) but did not want to break it too badly.

But if we are to use console.something(), and we are not differentiating between "error levels", then console.error() seems most appropriate given its ubiquitous use as a log to stderr without a presumed severity.

I like no-op default. If we’re concerned about performance, sometimes undefined and if (log) log() is considerably faster. I say sometimes because other times, branch prediction and inlining come to roost.

[kriskowal]

It would be helpful to separate the the log feature commit from the types refactor commit, to keep a record in the commit history, not confuse the conventional commit automation, and also leave us safer in case one or the other commit has to be reverted.

I prefer the noöp default logger over console.* given that Node.js and the web give the console inconsistent treatment.

Use your own discretion regarding all comments.

[kriskowal]

I do not mind this warning being silenced by default (noop log). There are legitimate cases.

[kriskowal]

I think the variadic args can be more constrained, possibly to types expressible in JSON only, though I acknowledge that the signature of JSON.parse returns any and, if it can’t be held to a higher standard, why should we hold ourselves that high. In any case, your discretion whether to punt.

[boneskull]

my take is this: if we end up emitting something that JSON.stringify cannot stringify and it breaks somebody, then we can worry about it (since we'd need to implement multiple types to handle it). at that point we can also decide whether to force the data to be immutable or a deep clone or a struct-like object or something (if leaking mutable objects is indeed a concern).

afaik the only thing that JSON.stringify would ever tantrum about is an object containing circular references. oftentimes, logging implementations will guard against circular refs anyway.

[boneskull]

• I split into two commits and added a third modifying NEWS.md
• I chose to use a NOP as the default logger