fix(sys): tweak NodeLazyRequire logic around too-high-versions errors (#3347)

alicewriteswrongs · web-flow · commit 9bfef1ad4637 · 2022-04-28T14:17:55.000-04:00
this changes the logic that we use in the `NodeLazyRequire` class to
error out in fewer situations. the changes are:

- the data structure for specifying our version ranges is changed from
  `[string, string]` (`[minVersion, recommendedVersion]`) to an object
  with `minVersion` `recommendedVersion` required properties and an
  optional `maxVersion` property.
- in the `ensure` method on `NodeLazyRequire` we check if `maxVersion`
  is defined on a given version range requirement.
    - If so, we check that the installed version is greater than or
      equal to `minVersion` and less than the major version of
      `maxVersion`.
    - If not, we just check that `minVersion &lt;= installedVersion`

this should give us the flexibility to mark certain versions of packages
as incompatible (for instance jest@28) without having to do that for all
packages that we lazy require. this is helpful because for most of them
we just want to set a minimum version and don't have a need for an
enforced maximum version.
diff --git a/src/sys/node/node-lazy-require.ts b/src/sys/node/node-lazy-require.ts
@@ -5,24 +5,34 @@ import fs from 'graceful-fs';
 import path from 'path';
 import satisfies from 'semver/functions/satisfies';
 import major from 'semver/functions/major';
+import semverLte from 'semver/functions/lte';
 
 /**
- * The version range that we support for a given package
- * [0] is the lower end, while [1] is the higher end.
- *
- * These strings should be standard semver strings.
+ * The version range that we support for a given package. The strings should be
+ * standard semver strings.
  */
-type NodeVersionRange = [string, string];
+interface DepVersionRange {
+  minVersion: string;
+  recommendedVersion: string;
+  /**
+   * Max version is optional because we aren't always worried about upgrades.
+   * This should be set for packages where major version upgrades have
+   * historically caused problems, or when we've identified a specific issue
+   * that requires us to stay at or below a certain version. Note that
+   * `NodeLazyRequire.ensure` only checks the major version.
+   */
+  maxVersion?: string;
+}
 
 /**
- * A manifest for lazily-loaded dependencies, mapping dependency names
- * to version ranges.
+ * A manifest for lazily-loaded dependencies, mapping dependency names to
+ * version ranges.
  */
-type LazyDependencies = Record<string, NodeVersionRange>;
+export type LazyDependencies = Record<string, DepVersionRange>;
 
 /**
- * Lazy requirer for Node, with functionality for specifying version ranges
- * and returning diagnostic errors if requirements aren't met.
+ * Lazy requirer for Node, with functionality for specifying version ranges and
+ * returning diagnostic errors if requirements aren't met.
  */
 export class NodeLazyRequire implements d.LazyRequire {
   private ensured = new Set<string>();
@@ -36,36 +46,44 @@ export class NodeLazyRequire implements d.LazyRequire {
   constructor(private nodeResolveModule: NodeResolveModule, private lazyDependencies: LazyDependencies) {}
 
   /**
-   * Ensure that a dependency within our supported range is installed in the current
-   * environment. This function will check all the dependency requirements passed in when
-   * the class is instantiated and return diagnostics if there are any issues.
+   * Ensure that a dependency within our supported range is installed in the
+   * current environment. This function will check all the dependency
+   * requirements passed in when the class is instantiated and return
+   * diagnostics if there are any issues.
    *
-   * @param fromDir the directory from which we'll attempt to resolve the dependencies, typically
-   * this will be project's root directory.
-   * @param ensureModuleIds an array of module names whose versions we're going to check
-   * @returns a Promise holding diagnostics if any of the dependencies either were not
-   * resolved _or_ did not meet our version requirements.
+   * @param fromDir the directory from which we'll attempt to resolve the
+   * dependencies, typically this will be project's root directory.
+   * @param ensureModuleIds an array of module names whose versions we're going
+   * to check
+   * @returns a Promise holding diagnostics if any of the dependencies either
+   * were not resolved _or_ did not meet our version requirements.
    */
   async ensure(fromDir: string, ensureModuleIds: string[]): Promise<d.Diagnostic[]> {
     const diagnostics: d.Diagnostic[] = [];
     const problemDeps: string[] = [];
 
     ensureModuleIds.forEach((ensureModuleId) => {
       if (!this.ensured.has(ensureModuleId)) {
-        const [minVersion, maxVersion] = this.lazyDependencies[ensureModuleId];
+        const { minVersion, recommendedVersion, maxVersion } = this.lazyDependencies[ensureModuleId];
 
         try {
           const pkgJsonPath = this.nodeResolveModule.resolveModule(fromDir, ensureModuleId);
           const installedPkgJson: d.PackageJsonData = JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8'));
 
-          if (satisfies(installedPkgJson.version, `${minVersion} - ${major(maxVersion)}.x`)) {
+          const installedVersionIsGood = maxVersion
+            ? // if maxVersion, check that `minVersion <= installedVersion <= maxVersion`
+              satisfies(installedPkgJson.version, `${minVersion} - ${major(maxVersion)}.x`)
+            : // else, just check that `minVersion <= installedVersion`
+              semverLte(minVersion, installedPkgJson.version);
+
+          if (installedVersionIsGood) {
             this.ensured.add(ensureModuleId);
             return;
           }
         } catch (e) {}
         // if we get here we didn't get to the `return` above, so either 1) there was some error
         // reading the package.json or 2) the version wasn't in our specified version range.
-        problemDeps.push(`${ensureModuleId}@${maxVersion}`);
+        problemDeps.push(`${ensureModuleId}@${recommendedVersion}`);
       }
     });
 
diff --git a/src/sys/node/node-sys.ts b/src/sys/node/node-sys.ts
@@ -588,14 +588,13 @@ export function createNodeSys(c: { process?: any } = {}) {
   const nodeResolve = new NodeResolveModule();
 
   sys.lazyRequire = new NodeLazyRequire(nodeResolve, {
-    // [minimumVersion, recommendedVersion]
-    '@types/jest': ['24.9.1', '27.0.3'],
-    jest: ['24.9.0', '27.4.5'],
-    'jest-cli': ['24.9.0', '27.4.5'],
-    pixelmatch: ['4.0.2', '4.0.2'],
-    puppeteer: ['1.19.0', '10.0.0'],
-    'puppeteer-core': ['1.19.0', '5.2.1'],
-    'workbox-build': ['4.3.1', '4.3.1'],
+    '@types/jest': { minVersion: '24.9.1', recommendedVersion: '27.0.3', maxVersion: '27.0.0' },
+    jest: { minVersion: '24.9.1', recommendedVersion: '27.0.3', maxVersion: '27.0.0' },
+    'jest-cli': { minVersion: '24.9.0', recommendedVersion: '27.4.5', maxVersion: '27.0.0' },
+    pixelmatch: { minVersion: '4.0.2', recommendedVersion: '4.0.2' },
+    puppeteer: { minVersion: '1.19.0', recommendedVersion: '10.0.0' },
+    'puppeteer-core': { minVersion: '1.19.0', recommendedVersion: '5.2.1' },
+    'workbox-build': { minVersion: '4.3.1', recommendedVersion: '4.3.1' },
   });
 
   prcs.on('SIGINT', runInterruptsCallbacks);
diff --git a/src/sys/node/test/node-lazy-require.spec.ts b/src/sys/node/test/node-lazy-require.spec.ts
@@ -1,4 +1,4 @@
-import { NodeLazyRequire } from '../node-lazy-require';
+import { LazyDependencies, NodeLazyRequire } from '../node-lazy-require';
 import { buildError } from '@utils';
 import { NodeResolveModule } from '../node-resolve-module';
 import fs from 'graceful-fs';
@@ -21,45 +21,63 @@ describe('node-lazy-require', () => {
         readFSMock.mockClear();
       });
 
-      function setup() {
+      const jestTestRange = (maxVersion = '38.0.1'): LazyDependencies => ({
+        jest: {
+          minVersion: '2.0.7',
+          recommendedVersion: '36.0.1',
+          maxVersion,
+        },
+      });
+
+      function setup(versionRange: LazyDependencies) {
         const resolveModule = new NodeResolveModule();
-        const nodeLazyRequire = new NodeLazyRequire(resolveModule, {
-          jest: ['2.0.7', '38.0.1'],
-        });
+        const nodeLazyRequire = new NodeLazyRequire(resolveModule, versionRange);
         return nodeLazyRequire;
       }
 
       it.each(['2.0.7', '10.10.10', '38.0.1', '38.0.2', '38.5.17'])(
         'should not error if installed package has a suitable major version (%p)',
         async (testVersion) => {
-          const nodeLazyRequire = setup();
+          const nodeLazyRequire = setup(jestTestRange());
+          readFSMock.mockReturnValue(mockPackageJson(testVersion));
+          let diagnostics = await nodeLazyRequire.ensure('.', ['jest']);
+          expect(diagnostics.length).toBe(0);
+        }
+      );
+
+      it.each(['2.0.7', '10.10.10', '36.0.1', '38.0.2', '38.5.17'])(
+        'should never error with versions above minVersion if there is no maxVersion supplied (%p)',
+        async (testVersion) => {
+          const nodeLazyRequire = setup(jestTestRange(undefined));
           readFSMock.mockReturnValue(mockPackageJson(testVersion));
           let diagnostics = await nodeLazyRequire.ensure('.', ['jest']);
           expect(diagnostics.length).toBe(0);
         }
       );
 
-      it('should error if the installed version of a package is too low', async () => {
-        const nodeLazyRequire = setup();
+      it.each(['38', undefined])('should error w/ installed version too low and maxVersion=%p', async (maxVersion) => {
+        const range = jestTestRange(maxVersion);
+        const nodeLazyRequire = setup(range);
         readFSMock.mockReturnValue(mockPackageJson('1.1.1'));
         let [error] = await nodeLazyRequire.ensure('.', ['jest']);
         expect(error).toEqual({
           ...buildError([]),
           header: 'Please install supported versions of dev dependencies with either npm or yarn.',
-          messageText: 'npm install --save-dev jest@38.0.1',
+          messageText: `npm install --save-dev jest@${range.jest.recommendedVersion}`,
         });
       });
 
       it.each(['100.1.1', '38.0.1-alpha.0'])(
         'should error if the installed version of a package is too high (%p)',
         async (version) => {
-          const nodeLazyRequire = setup();
+          const range = jestTestRange();
+          const nodeLazyRequire = setup(range);
           readFSMock.mockReturnValue(mockPackageJson(version));
           let [error] = await nodeLazyRequire.ensure('.', ['jest']);
           expect(error).toEqual({
             ...buildError([]),
             header: 'Please install supported versions of dev dependencies with either npm or yarn.',
-            messageText: 'npm install --save-dev jest@38.0.1',
+            messageText: `npm install --save-dev jest@${range.jest.recommendedVersion}`,
           });
         }
       );
