chore: peer review changes

endojs · Jan 4, 2024 · ee8debd · ee8debd
1 parent bea662f
commit ee8debd
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 22 deletions.
diff --git a/packages/compartment-mapper/src/node-modules.js b/packages/compartment-mapper/src/node-modules.js
@@ -42,18 +42,18 @@
  * @typedef {Record<string, {spec: string, alias: string}>} CommonDependencyDescriptors
  */
 
+import { pathCompare } from './compartment-map.js';
 import { inferExportsAndAliases } from './infer-exports.js';
-import { searchDescriptor } from './search.js';
 import { parseLocatedJson } from './json.js';
-import { unpackReadPowers } from './powers.js';
+import { join } from './node-module-specifier.js';
+import { assertPolicy } from './policy-format.js';
 import {
-  getPolicyForPackage,
   ATTENUATORS_COMPARTMENT,
   dependencyAllowedByPolicy,
+  getPolicyForPackage,
 } from './policy.js';
-import { join } from './node-module-specifier.js';
-import { pathCompare } from './compartment-map.js';
-import { assertPolicy } from './policy-format.js';
+import { unpackReadPowers } from './powers.js';
+import { searchDescriptor } from './search.js';
 
 const { assign, create, keys, values } = Object;
 
@@ -566,7 +566,7 @@ const graphPackages = async (
  * @param {Graph} graph
  * @param {Set<string>} tags - build tags about the target environment
  * for selecting relevant exports, e.g., "browser" or "node".
- * @param {object|undefined} policy
+ * @param {import('./types.js').Policy} [policy]
  * @returns {CompartmentMapDescriptor}
  */
 const translateGraph = (
@@ -626,13 +626,14 @@ const translateGraph = (
         const localPath = join(dependencyName, exportPath);
         if (
           !policy ||
-          dependencyAllowedByPolicy(
-            {
-              name,
-              path,
-            },
-            /** @type {import('./types.js').PackagePolicy} */ (packagePolicy),
-          )
+          (packagePolicy &&
+            dependencyAllowedByPolicy(
+              {
+                name,
+                path,
+              },
+              packagePolicy,
+            ))
         ) {
           moduleDescriptors[localPath] = {
             compartment: packageLocation,

diff --git a/packages/compartment-mapper/src/policy-format.js b/packages/compartment-mapper/src/policy-format.js
@@ -133,11 +133,11 @@ const isPolicyItem = item =>
  * @param {unknown} allegedPackagePolicy - Alleged `PackagePolicy` to test
  * @param {string} path - Path in the `Policy` object; used for error messages only
  * @param {string} [url] - URL of the policy file; used for error messages only
- * @returns {allegedPackagePolicy is import('./types.js').PackagePolicy}
+ * @returns {asserts allegedPackagePolicy is import('./types.js').PackagePolicy|undefined}
  */
 export const assertPackagePolicy = (allegedPackagePolicy, path, url) => {
   if (allegedPackagePolicy === undefined) {
-    return true;
+    return;
   }
   const inUrl = url ? ` in ${q(url)}` : '';
 
@@ -191,7 +191,6 @@ export const assertPackagePolicy = (allegedPackagePolicy, path, url) => {
         builtins,
       })}${inUrl}`,
     );
-  return true;
 };
 
 /**
@@ -200,11 +199,11 @@ export const assertPackagePolicy = (allegedPackagePolicy, path, url) => {
  * It also moonlights as a type guard.
  *
  * @param {unknown} allegedPolicy - Alleged `Policy` to test
- * @returns {allegedPolicy is import('./types.js').Policy}
+ * @returns {asserts allegedPolicy is import('./types.js').Policy|undefined}
  */
 export const assertPolicy = allegedPolicy => {
   if (allegedPolicy === undefined) {
-    return true;
+    return;
   }
   const policy = Object(allegedPolicy);
   assert(
@@ -232,5 +231,4 @@ export const assertPolicy = allegedPolicy => {
   for (const [key, value] of entries(resources)) {
     assertPackagePolicy(value, `policy.resources["${key}"]`);
   }
-  return true;
 };
diff --git a/packages/compartment-mapper/src/policy.js b/packages/compartment-mapper/src/policy.js
@@ -114,10 +114,27 @@ export const dependencyAllowedByPolicy = (namingKit, packagePolicy) => {
 /**
  * Returns the policy applicable to the canonicalName of the package
  *
- * @param {import('./types.js').PackageNamingKit} namingKit - a key in the policy resources spec is derived frm these
+ * @overload
+ * @param {import('./types.js').PackageNamingKit} namingKit - a key in the policy resources spec is derived from these
+ * @param {import('./types.js').Policy} policy - user supplied policy
+ * @returns {import('./types.js').PackagePolicy} packagePolicy if policy was specified
+ */
+
+/**
+ * Returns `undefined`
+ *
+ * @overload
+ * @param {import('./types.js').PackageNamingKit} namingKit - a key in the policy resources spec is derived from these
  * @param {import('./types.js').Policy} [policy] - user supplied policy
  * @returns {import('./types.js').PackagePolicy|undefined} packagePolicy if policy was specified
  */
+
+/**
+ * Returns the policy applicable to the canonicalName of the package
+ *
+ * @param {import('./types.js').PackageNamingKit} namingKit - a key in the policy resources spec is derived from these
+ * @param {import('./types.js').Policy} [policy] - user supplied policy
+ */
 export const getPolicyForPackage = (namingKit, policy) => {
   if (!policy) {
     return undefined;
@@ -149,7 +166,7 @@ const getGlobalsList = packagePolicy => {
   if (!packagePolicy?.globals) {
     return [];
   }
-  return entries(packagePolicy?.globals)
+  return entries(packagePolicy.globals)
     .filter(([_key, value]) => value)
     .map(([key, _vvalue]) => key);
 };