feat: review suggestions

packages/env-options/README.md

@@ -1 +1,75 @@
+# Parameterizing Modules with Environment Options
 
+JavaScript module semantics resists attempts to parameterize a module's
+initialization behavior. A module initializes in order according to
+the path by which it is first imported, and then the initialized module
+is reused by all the other times it is imported. Compartments give us
+the opportunity to bind the same import name to different imported
+modules, depending on the package/compartment doing the import. Compartments
+also address the difficulty of parameterizing a module's initialization
+logic, but not in a pleasant manner.
+
+A pleasant parameterization would be for a static module to be function-like
+with explicit parameters, and for the parameterization to be like
+calling the static module with parameters in order to derive from it a
+module instance. Compartments instead lets us parameterize the meaning
+of a module instance derived from a static module according to the
+three namespaces provided by the JavaScript semantics, affecting the
+meaning of a module instance.
+   * The global variable namespaces.
+      * The global scope, aliased to properties of the global object.
+        This is necessarily compartment-wide. In our
+        recommened usage pattern of one compartment per package,
+        each global would be package-wide. (See LavaMoat)
+      * The global lexical scope. The SES-shim compartments support
+        these both compartment-wide as well as per-module. But it is
+        not yet clear what we will propose in the Compartment proposal.
+   * The import namespace.
+   * The host hooks.
+
+This `@endo/env-options` package follows the Node precedent for
+finding Unix environment variable settings: looking for a
+global `process` object holding an `env` object,
+optionally holding a property with the same name as the option,
+whose value is the configuration setting of that option.
+
+```js
+import { makeEnvironmentCaptor } from '@endo/env-options';
+const { getEnvironmentOption } = makeEnvironmentCaptor(globalThis);
+const FooBarOption = getEnvironmentOption('FOO_BAR', 'absent');
+```
+
+The first argument to `getEnvironmentOption` is the name of the option.
+The value of `FooBarOption` would then be the value of
+`globalThis.process.env.FOO_BAR`, if present.
+If setting is either absent or `undefined`, the default `'absent'`
+would be used instead.
+
+In either case, reflecting Unix environment variable expectations,
+the resulting setting must be a string.
+This restriction also helps ensure that this channel is used only to pass data,
+not authority beyond the ability to read this global state.
+
+The `makeEnvironmentCaptor` function also returns a
+`getCapturedEnvironmentOptionNames` function for use to give feedback about
+which environment variables were actually read, for diagnostic purposes. The
+ses-shim `lockdown` contains code such as the following, to explain which
+environment variables were read to provide `lockdown` settings.
+
+```js
+import { makeEnvironmentCaptor } from '@endo/env-options';
+const {
+  getEnvironmentOption,
+  getCapturedEnvironmentOptionNames,
+} = makeEnvironmentCaptor(globalThis);
+...
+const capturedEnvironmentOptionNames = getCapturedEnvironmentOptionNames();
+if (capturedEnvironmentOptionNames.length > 0) {
+  console.warn(
+    `SES Lockdown using options from environment variables ${enJoin(
+      arrayMap(capturedEnvironmentOptionNames, q),
+      'and',
+    )}`,
+  );
+}
+```

packages/env-options/package.json

@@ -2,7 +2,7 @@
   "name": "@endo/env-options",
   "version": "0.1.0",
   "private": null,
-  "description": "Description forthcoming.",
+  "description": "Reading environment options.",
   "keywords": [],
   "author": "Endo contributors",
   "license": "Apache-2.0",

packages/env-options/src/environment-options.js

@@ -14,50 +14,7 @@ const Fail = (template, ...args) => {
 };
 
 /**
- * JavaScript module semantics resists attempts to parameterize a module's
- * initialization behavior. A module initializes in order according to
- * the path by which it is first imported, and then the initialized module
- * is reused by all the other times it is imported. Compartments give us
- * the opportunity to bind the same import name to different imported
- * modules, depending on the package/compartment doing the import. Compartments
- * also address the difficulty of parameterizing a module's initialization
- * logic, but not in a pleasant manner.
- *
- * A pleasant parameterization would be for a static module to be function-like
- * with explicit parameters, and for the parameterization to be like
- * calling the static module with parameters in order to derive from it a
- * module instance. Compartments instead lets us parameterize the meaning
- * of a module instance derived from a static module according to the
- * three namespaces provided by the JavaScript semantics, affecting the
- * meaning of a module instance.
- *    * The global variable namespaces.
- *       * The global scope, aliased to properties of the global object.
- *         This is necessarily compartment-wide, and therefore in our
- *         recommened usage pattern, package-wide.
- *       * The global lexical scope. The SES-shim compartments support
- *         these both compartment-wide as well as per-module. But it is
- *         not yet clear what we will propose in the Compartment proposal.
- *    * The import namespace.
- *    * The host hooks.
- *
- * This `environment-options.js` module looks for a setting of an
- * `optionName` parameter rooted in the global scope. If follows the Node
- * precedent for finding Unix environment variable settings, looking for a
- * global `process` object holding an `env` object,
- * optionally holding a property named for the `optionName` whose value is the
- * configuration setting of that option. For example, for the optionName
- * `FOO_BAR` it would look in
- * `globalThis.process.env.FOO_BAR`.
- *
- * If setting is either absent or `undefined`, that indicates that
- * this configuration option should have its default behavior, whatever that is.
- * Otherwise, reflecting Unix environment variables, the setting must be a
- * string. This also helps ensure that this channel is used only to pass data,
- * not authority beyond the ability to read this global state.
- */
-
-/**
- * makeEnvironmentCaptor provides a mechanism for getting environment
+ * `makeEnvironmentCaptor` provides a mechanism for getting environment
  * variables, if they are needed, and a way to catalog the names of all
  * the environment variables that were captured.
  *

packages/env-options/test/test-env-options.js

@@ -0,0 +1,37 @@
+import { test } from './prepare-test-env-ava.js';
+import { makeEnvironmentCaptor } from '../src/environment-options.js';
+
+test('test env options empty env', async t => {
+  const c1 = new Compartment();
+  const { getEnvironmentOption, getCapturedEnvironmentOptionNames } =
+    makeEnvironmentCaptor(c1.globalThis);
+
+  const c1foo = getEnvironmentOption('FOO', 'none');
+  t.is(c1foo, 'none');
+  t.deepEqual(getCapturedEnvironmentOptionNames(), []);
+});
+
+test('test env options present', t => {
+  const c2 = new Compartment({
+    process: {
+      env: {
+        FOO: 'bar',
+        BAD: ['not a string'],
+      },
+    },
+  });
+  const { getEnvironmentOption, getCapturedEnvironmentOptionNames } =
+    makeEnvironmentCaptor(c2.globalThis);
+
+  const c2foo = getEnvironmentOption('FOO', 'none');
+  t.is(c2foo, 'bar');
+  t.deepEqual(getCapturedEnvironmentOptionNames(), ['FOO']);
+
+  t.throws(() => getEnvironmentOption('BAD', 'none'), {
+    message:
+      'Environment option named "BAD", if present, must have a corresponding string value, got ["not a string"]',
+  });
+  t.throws(() => getEnvironmentOption('WORSE', ['none']), {
+    message: 'Environment option default setting ["none"] must be a string.',
+  });
+});

packages/eventual-send/package.json

@@ -29,6 +29,9 @@
     "url": "https://github.com/endojs/endo/issues"
   },
   "homepage": "https://github.com/endojs/endo#readme",
+  "dependencies": {
+    "@endo/env-options": "^0.1.0"
+  },
   "devDependencies": {
     "@endo/lockdown": "^0.1.28",
     "@endo/ses-ava": "^0.2.40",

packages/eventual-send/src/track-turns.js

@@ -1,7 +1,8 @@
 /* global globalThis */
 // @ts-nocheck
+import { makeEnvironmentCaptor } from '@endo/env-options';
 
-const { freeze } = Object;
+const { getEnvironmentOption } = makeEnvironmentCaptor(globalThis);
 
 // NOTE: We can't import these because they're not in scope before lockdown.
 // import { assert, details as X, Fail } from '@agoric/assert';
@@ -17,22 +18,18 @@ let hiddenPriorError;
 let hiddenCurrentTurn = 0;
 let hiddenCurrentEvent = 0;
 
-// TODO Use environment-options.js currently in ses/src after factoring it out
-// to a new package.
-const env = (globalThis.process || {}).env || {};
+const DEBUG = getEnvironmentOption('DEBUG', '');
 
 // Turn on if you seem to be losing error logging at the top of the event loop
-const VERBOSE = (env.DEBUG || '').split(':').includes('track-turns');
+const VERBOSE = DEBUG.split(':').includes('track-turns');
 
-const validOptionValues = freeze([undefined, 'enabled', 'disabled']);
-
-// Track-turns is enabled by default and can be disabled by an environment
+// Track-turns is disabled by default and can be enabled by an environment
 // option.
-const envOptionValue = env.TRACK_TURNS;
-if (!validOptionValues.includes(envOptionValue)) {
-  throw TypeError(`unrecognized TRACK_TURNS ${JSON.stringify(envOptionValue)}`);
+const TRACK_TURNS = getEnvironmentOption('TRACK_TURNS', 'disabled');
+if (TRACK_TURNS !== 'enabled' && TRACK_TURNS !== 'disabled') {
+  throw TypeError(`unrecognized TRACK_TURNS ${JSON.stringify(TRACK_TURNS)}`);
 }
-const ENABLED = (envOptionValue || 'disabled') === 'enabled';
+const ENABLED = (TRACK_TURNS || 'disabled') === 'enabled';
 
 // We hoist the following functions out of trackTurns() to discourage the
 // closures from holding onto 'args' or 'func' longer than necessary,

packages/eventual-send/test/prepare-test-env-ava.js

@@ -1,4 +1,4 @@
-/* global process */
+/* global globalThis */
 
 import '@endo/lockdown/commit-debug.js';
 
@@ -10,4 +10,5 @@ export * from 'ava';
 /** @type {typeof rawTest} */
 export const test = wrapTest(rawTest);
 
-process.env.TRACK_TURNS = 'enabled';
+const env = (globalThis.process || {}).env || {};
+env.TRACK_TURNS = 'enabled';

packages/exo/package.json

@@ -32,6 +32,7 @@
     "test": "ava"
   },
   "dependencies": {
+    "@endo/env-options": "^0.1.0",
     "@endo/far": "^0.2.18",
     "@endo/patterns": "^0.2.2"
   },

packages/exo/src/exo-makers.js

@@ -1,19 +1,17 @@
 /* global globalThis */
 /// <reference types="ses"/>
+import { makeEnvironmentCaptor } from '@endo/env-options';
 import { objectMap } from '@endo/patterns';
 
 import { defendPrototype, defendPrototypeKit } from './exo-tools.js';
 
 const { create, seal, freeze, defineProperty } = Object;
 
-// TODO Use environment-options.js currently in ses/src after factoring it out
-// to a new package.
-const env = (globalThis.process || {}).env || {};
+const { getEnvironmentOption } = makeEnvironmentCaptor(globalThis);
+const DEBUG = getEnvironmentOption('DEBUG', '');
 
 // Turn on to give each exo instance its own toStringTag value.
-const LABEL_INSTANCES = (env.DEBUG || '')
-  .split(',')
-  .includes('label-instances');
+const LABEL_INSTANCES = DEBUG.split(',').includes('label-instances');
 
 const makeSelf = (proto, instanceCount) => {
   const self = create(proto);

packages/exo/test/prepare-test-env-ava-label-instances.js

@@ -2,8 +2,6 @@
 
 export * from './prepare-test-env-ava.js';
 
-// TODO Use environment-options.js currently in ses/src after factoring it out
-// to a new package.
 const env = (globalThis.process || {}).env || {};
 
 env.DEBUG = 'label-instances';