fixup! review suggestion

Author: erights

packages/non-trapping-shim/README.md

@@ -11,11 +11,24 @@ This package is currently organized internally as a ponyfill, and a shim based o
 
 See https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md for guidance on how to prepare for the changes that will be introduced by this proposal.
 
+The shim needs to be imported ***early***, in particular before other code might sample of freeze the primordials that this shim would modify or replace. Even though the `@endo/non-trapping-shim` package does not export a ponyfill, to uphold the convention on side-effecting exports, it does not export the shim as the package's default export. Rather, you need to import it as:
+
+```js
+import '@endo/non-trapping-shim/shim.js';
+```
+
 ## Opt-in env-option `SES_NON_TRAPPING_SHIM`
 
 To cope with various compat problems in linking code that uses or assumes this shim to code that does not, we have made this shim opt-in via the env-option `SES_NON_TRAPPING_SHIM`. This has two settings, `'enabled'` and the default `'disabled'`. As with all env options, this is represented at the property `process.env.SES_NON_TRAPPING_SHIM`, which typically represents the environment variable `SES_NON_TRAPPING_SHIM`. Thus, if nothing else sets `process.env.SES_NON_TRAPPING_SHIM`, you can opt-in at the shell level by
 ```sh
 $ export SES_NON_TRAPPING_SHIM=enabled
 ```
 
+The shim senses the setting of this env-option at the time it is imported, so any desired setting will need to happen earlier. Using a genuine environment variable does this. Otherwise, as a convenience, this package also exports a module to opt-in by setting `process.env.SES_NON_TRAPPING_SHIM`. To use this convenience, import the exported `prepare-enable-shim.js` before importing the exported shim itself:
+
+```js
+import '@endo/non-trapping-shim/prepare-enable-shim.js';
+import '@endo/non-trapping-shim/shim.js';
+```
+
 When not opted into, importing the shim has no effect.

packages/non-trapping-shim/src/non-trapping-pony.js

@@ -20,18 +20,17 @@ const isPrimitive = specimen => OriginalObject(specimen) !== specimen;
  * `Reflect.isNonTrapping`.
  *
  * @param {any} specimen
- * @param {boolean} shouldThrow
  * @returns {boolean}
  */
-const isNonTrappingInternal = (specimen, shouldThrow) => {
+const isNonTrappingInternal = specimen => {
   if (nonTrappingSet.has(specimen)) {
     return true;
   }
   if (!proxyHandlerMap.has(specimen)) {
     return false;
   }
   const [target, handler] = proxyHandlerMap.get(specimen);
-  if (isNonTrappingInternal(target, shouldThrow)) {
+  if (isNonTrappingInternal(target)) {
     nonTrappingSet.add(specimen);
     return true;
   }
@@ -40,14 +39,11 @@ const isNonTrappingInternal = (specimen, shouldThrow) => {
     return false;
   }
   const result = apply(trap, handler, [target]);
-  const ofTarget = isNonTrappingInternal(target, shouldThrow);
+  const ofTarget = isNonTrappingInternal(target);
   if (result !== ofTarget) {
-    if (shouldThrow) {
-      throw TypeError(
-        `'isNonTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`,
-      );
-    }
-    return false;
+    throw TypeError(
+      `'isNonTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`,
+    );
   }
   if (result) {
     nonTrappingSet.add(specimen);
@@ -60,10 +56,9 @@ const isNonTrappingInternal = (specimen, shouldThrow) => {
  * `Reflect.suppressTrapping`.
  *
  * @param {any} specimen
- * @param {boolean} shouldThrow
  * @returns {boolean}
  */
-const suppressTrappingInternal = (specimen, shouldThrow) => {
+const suppressTrappingInternal = specimen => {
   if (nonTrappingSet.has(specimen)) {
     return true;
   }
@@ -73,27 +68,24 @@ const suppressTrappingInternal = (specimen, shouldThrow) => {
     return true;
   }
   const [target, handler] = proxyHandlerMap.get(specimen);
-  if (isNonTrappingInternal(target, shouldThrow)) {
+  if (isNonTrappingInternal(target)) {
     nonTrappingSet.add(specimen);
     return true;
   }
   const trap = handler.suppressTrapping;
   if (trap === undefined) {
-    const result = suppressTrappingInternal(target, shouldThrow);
+    const result = suppressTrappingInternal(target);
     if (result) {
       nonTrappingSet.add(specimen);
     }
     return result;
   }
   const result = apply(trap, handler, [target]);
-  const ofTarget = isNonTrappingInternal(target, shouldThrow);
+  const ofTarget = isNonTrappingInternal(target);
   if (result !== ofTarget) {
-    if (shouldThrow) {
-      throw TypeError(
-        `'suppressTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`,
-      );
-    }
-    return false;
+    throw TypeError(
+      `'suppressTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`,
+    );
   }
   if (result) {
     nonTrappingSet.add(specimen);
@@ -106,13 +98,13 @@ export const extraReflectMethods = freeze({
     if (isPrimitive(target)) {
       throw TypeError('Reflect.isNonTrapping called on non-object');
     }
-    return isNonTrappingInternal(target, false);
+    return isNonTrappingInternal(target);
   },
   suppressTrapping(target) {
     if (isPrimitive(target)) {
       throw TypeError('Reflect.suppressTrapping called on non-object');
     }
-    return suppressTrappingInternal(target, false);
+    return suppressTrappingInternal(target);
   },
 });
 
@@ -121,13 +113,13 @@ export const extraObjectMethods = freeze({
     if (isPrimitive(target)) {
       return true;
     }
-    return isNonTrappingInternal(target, true);
+    return isNonTrappingInternal(target);
   },
   suppressTrapping(target) {
     if (isPrimitive(target)) {
       return target;
     }
-    if (suppressTrappingInternal(target, true)) {
+    if (suppressTrappingInternal(target)) {
       return target;
     }
     throw TypeError('suppressTrapping trap returned falsy');
@@ -198,7 +190,7 @@ const metaHandler = freeze({
      * @param {any[]} rest
      */
     const trapPlus = freeze((target, ...rest) => {
-      if (isNonTrappingInternal(target, true)) {
+      if (isNonTrappingInternal(target)) {
         defineProperty(handlerPlus, trapName, {
           value: undefined,
           writable: false,
@@ -284,7 +276,7 @@ ProxyPlus.revocable = (target, handler) => {
   return {
     proxy,
     revoke() {
-      if (isNonTrappingInternal(target, true)) {
+      if (isNonTrappingInternal(target)) {
         throw TypeError('Cannot revoke non-trapping proxy');
       }
       revoke();