fix(ruleset-migrator): resolve npm packages correctly (#2022)

packages/ruleset-migrator/package.json

@@ -33,7 +33,8 @@
     "ast-types": "0.14.2",
     "astring": "^1.7.5",
     "reserved": "0.1.2",
-    "tslib": "^2.3.1"
+    "tslib": "^2.3.1",
+    "validate-npm-package-name": "3.0.0"
   },
   "devDependencies": {
     "@stoplight/spectral-core": "^1.1.0",

packages/ruleset-migrator/src/__tests__/ruleset.jest.test.ts

@@ -0,0 +1,93 @@
+import * as path from '@stoplight/path';
+import * as fs from 'fs';
+import { migrateRuleset } from '../index';
+
+describe('migrator', () => {
+  describe('node_modules resolution', () => {
+    const cwd = path.join(__dirname, '.cache');
+    const name = 'my-npm-ruleset';
+
+    beforeAll(async () => {
+      const pkgRoot = path.join(cwd, 'node_modules', name);
+      await fs.promises.mkdir(pkgRoot, { recursive: true });
+      await fs.promises.writeFile(
+        path.join(pkgRoot, 'package.json'),
+        JSON.stringify({
+          name,
+          main: './index.json',
+        }),
+      );
+      await fs.promises.writeFile(
+        path.join(pkgRoot, 'index.json'),
+        JSON.stringify({
+          functions: ['test'],
+          rules: {
+            test: {
+              given: '$',
+              then: {
+                function: 'test',
+              },
+            },
+          },
+        }),
+      );
+    });
+
+    afterAll(async () => {
+      await fs.promises.rmdir(cwd, { recursive: true });
+    });
+
+    it('should be supported', async () => {
+      await fs.promises.writeFile(
+        path.join(cwd, 'my-ruleset.json'),
+        JSON.stringify({
+          extends: [name],
+          formats: ['oas2'],
+          rules: {
+            rule: {
+              then: {
+                given: '$',
+                function: 'truthy',
+              },
+            },
+          },
+        }),
+      );
+
+      expect(
+        await migrateRuleset(path.join(cwd, 'my-ruleset.json'), {
+          format: 'esm',
+          fs: {
+            promises: {
+              readFile: fs.promises.readFile,
+            },
+          },
+        }),
+      ).toEqual(`import {oas2} from "@stoplight/spectral-formats";
+import {truthy} from "@stoplight/spectral-functions";
+import test from "${cwd}/node_modules/my-npm-ruleset/functions/test.js";
+export default {
+  "extends": [{
+    "rules": {
+      "test": {
+        "given": "$",
+        "then": {
+          "function": test
+        }
+      }
+    }
+  }],
+  "formats": [oas2],
+  "rules": {
+    "rule": {
+      "then": {
+        "given": "$",
+        "function": truthy
+      }
+    }
+  }
+};
+`);
+    });
+  });
+});

packages/ruleset-migrator/src/__tests__/ruleset.test.ts

@@ -1,10 +1,11 @@
 import { fs } from 'memfs';
-import * as path from 'path';
+import * as path from '@stoplight/path';
 import * as prettier from 'prettier/standalone';
 import * as parserBabel from 'prettier/parser-babel';
 import { Ruleset } from '@stoplight/spectral-core';
 import { DiagnosticSeverity } from '@stoplight/types';
 import * as fetchMock from 'fetch-mock';
+import { serveAssets } from '@stoplight/spectral-test-utils';
 
 import { migrateRuleset } from '..';
 import fixtures from './__fixtures__/.cache/index.json';
@@ -172,10 +173,23 @@ describe('migrator', () => {
 
   describe('custom npm registry', () => {
     it('should be supported', async () => {
+      serveAssets({
+        'https://unpkg.com/custom-npm-ruleset': {
+          functions: ['test'],
+          rules: {
+            rule2: {
+              then: {
+                given: '$',
+                function: 'test',
+              },
+            },
+          },
+        },
+      });
       await fs.promises.writeFile(
         path.join(cwd, 'custom-npm-provider.json'),
         JSON.stringify({
-          extends: 'spectral:asyncapi',
+          extends: ['custom-npm-ruleset'],
           formats: ['oas2'],
           rules: {
             rule: {
@@ -195,9 +209,18 @@ describe('migrator', () => {
         }),
       ).toEqual(`import {oas2} from "https://unpkg.com/@stoplight/spectral-formats";
 import {truthy} from "https://unpkg.com/@stoplight/spectral-functions";
-import {asyncapi} from "https://unpkg.com/@stoplight/spectral-rulesets";
+import test from "https://unpkg.com/custom-npm-ruleset/functions/test.js";
 export default {
-  "extends": asyncapi,
+  "extends": [{
+    "rules": {
+      "rule2": {
+        "then": {
+          "given": "$",
+          "function": test
+        }
+      }
+    }
+  }],
   "formats": [oas2],
   "rules": {
     "rule": {

packages/ruleset-migrator/src/index.ts

@@ -8,13 +8,10 @@ import { Scope, Tree } from './tree';
 import { builders as b, namedTypes } from 'ast-types';
 import { ExpressionKind } from 'ast-types/gen/kinds';
 import { assertRuleset } from './validation';
-import requireResolve from './requireResolve';
 import { Ruleset } from './validation/types';
 
 async function read(filepath: string, fs: MigrationOptions['fs'], fetch: Fetch): Promise<Ruleset> {
-  const input = isURL(filepath)
-    ? await (await fetch(filepath)).text()
-    : await fs.promises.readFile(requireResolve?.(filepath) ?? filepath, 'utf8');
+  const input = isURL(filepath) ? await (await fetch(filepath)).text() : await fs.promises.readFile(filepath, 'utf8');
 
   const { data: ruleset } =
     extname(filepath) === '.json'
@@ -40,11 +37,13 @@ export async function migrateRuleset(filepath: string, opts: MigrationOptions):
   const hooks = new Set<Hook>();
   const ctx: TransformerCtx = {
     cwd,
+    filepath,
     tree,
     opts: {
       fetch,
       ...opts,
     },
+    npmRegistry: npmRegistry ?? null,
     hooks,
     read,
   };

packages/ruleset-migrator/src/transformers/extends.ts

@@ -22,15 +22,15 @@ async function processExtend(
     return ctx.tree.addImport(REPLACEMENTS[name], '@stoplight/spectral-rulesets');
   }
 
-  const filepath = ctx.tree.resolveModule(name, ctx.cwd);
+  const filepath = ctx.tree.resolveModule(name, ctx, 'ruleset');
 
   if (KNOWN_JS_EXTS.test(path.extname(filepath))) {
     return ctx.tree.addImport(`${path.basename(filepath, true)}_${path.extname(filepath)}`, filepath, true);
   }
 
   return await process(await ctx.read(filepath, ctx.opts.fs, ctx.opts.fetch), {
     ...ctx,
-    cwd: path.dirname(filepath),
+    filepath,
     tree: ctx.tree.fork(),
   });
 }

packages/ruleset-migrator/src/transformers/functions.ts

@@ -16,8 +16,9 @@ const transformer: Transformer = function (hooks) {
         for (const fn of functions) {
           assertString(fn);
           const resolved = ctx.tree.resolveModule(
-            `${fn}.js`,
-            path.join(ctx.cwd, typeof functionsDir === 'string' ? functionsDir : 'functions'),
+            path.join('./', typeof functionsDir === 'string' ? functionsDir : 'functions', `./${fn}.js`),
+            ctx,
+            'function',
           );
           const fnName = path.basename(resolved, true);
           const identifier = ctx.tree.addImport(fnName, resolved, true);

packages/ruleset-migrator/src/tree/index.ts

@@ -1,12 +1,14 @@
 import { namedTypes, builders as b } from 'ast-types';
 import * as path from '@stoplight/path';
 import * as astring from 'astring';
-import { MigrationOptions } from '../types';
+import { MigrationOptions, TransformerCtx } from '../types';
 import { commonjs } from './commonjs';
 import { esm } from './esm';
 import { IModule } from './types';
 import requireResolve from '../requireResolve';
 import { Scope } from './scope';
+import { isPackageImport } from '../utils/isPackageImport';
+import { isKnownNpmRegistry } from '../utils/isKnownNpmRegistry';
 
 export { Scope };
 
@@ -31,7 +33,7 @@ export class Tree {
 
   readonly #npmRegistry;
   readonly #module: IModule;
-  readonly #localPaths = new Set<string>();
+  readonly #resolvedPaths = new Set<string>();
 
   public ruleset?: namedTypes.ObjectExpression;
   public scope: Scope;
@@ -82,7 +84,7 @@ export class Tree {
           .sort(sortImports)
           .flatMap(([source, identifiers]) => {
             const resolvedSource =
-              this.#npmRegistry !== null && !this.#localPaths.has(source)
+              this.#npmRegistry !== null && !this.#resolvedPaths.has(source) && !source.startsWith(this.#npmRegistry)
                 ? path.join(this.#npmRegistry, source)
                 : source;
 
@@ -121,10 +123,28 @@ export class Tree {
     return b.identifier(uniqName);
   }
 
-  public resolveModule(identifier: string, cwd: string): string {
-    const resolved = path.isURL(identifier) ? identifier : requireResolve?.(identifier) ?? path.join(cwd, identifier);
-    if (resolved.startsWith(cwd)) {
-      this.#localPaths.add(resolved);
+  public resolveModule(identifier: string, ctx: TransformerCtx, kind: 'function' | 'ruleset'): string {
+    let resolved: string;
+    if (path.isURL(identifier) || path.isAbsolute(identifier)) {
+      resolved = identifier;
+      this.#resolvedPaths.add(identifier);
+    } else if (kind === 'ruleset' && isPackageImport(identifier)) {
+      resolved =
+        ctx.npmRegistry !== null
+          ? path.join(ctx.npmRegistry, identifier)
+          : requireResolve?.(identifier, { paths: [ctx.cwd] }) ?? path.join(ctx.cwd, identifier);
+    } else if (
+      (ctx.npmRegistry !== null && ctx.filepath.startsWith(ctx.npmRegistry)) ||
+      isKnownNpmRegistry(ctx.filepath)
+    ) {
+      // npm repos need a different resolution
+      // they should have the following pattern
+      // <origin>/<pkg-name>
+      // <origin>/<pkg-name>/<asset> where asset can be a custom fn, etc.
+      resolved = path.join(ctx.filepath, identifier);
+    } else {
+      resolved = path.join(ctx.filepath, '..', identifier);
+      this.#resolvedPaths.add(resolved);
     }
 
     return resolved;

packages/ruleset-migrator/src/types.ts

@@ -37,6 +37,8 @@ export type TransformerCtx = {
     fetch: Fetch;
   };
   readonly hooks: Set<Hook>;
-  cwd: string;
+  readonly cwd: string;
+  readonly filepath: string;
+  readonly npmRegistry: string | null;
   read(filepath: string, fs: MigrationOptions['fs'], fetch: Fetch): Promise<Ruleset>;
 };

packages/ruleset-migrator/src/utils/__tests__/isKnownNpmRegistry.spec.ts

@@ -0,0 +1,19 @@
+import { isKnownNpmRegistry } from '../isKnownNpmRegistry';
+
+describe('isKnownNmRegistry util', () => {
+  it.each([
+    'https://unpkg.com/spectral-aws-apigateway-ruleset',
+    'https://unpkg.com/spectral-aws-apigateway-ruleset/functions/draft4.js',
+    'https://cdn.skypack.dev/@stoplight/spectral-core',
+  ])('given recognized %s registry, should return true', input => {
+    expect(isKnownNpmRegistry(input)).toBe(true);
+  });
+
+  it.each([
+    'ftp://unpkg.com/spectral-aws-apigateway-ruleset',
+    '/nimma/legacy',
+    'https://baz.unpkg.com/spectral-aws-apigateway-ruleset',
+  ])('given unrecognized %s entity, should return false', input => {
+    expect(isKnownNpmRegistry(input)).toBe(false);
+  });
+});

packages/ruleset-migrator/src/utils/__tests__/isPackageImport.spec.ts

@@ -0,0 +1,23 @@
+import { isPackageImport } from '../isPackageImport';
+
+describe('isPackageImport util', () => {
+  it.each([
+    'nimma/legacy',
+    'nimma',
+    'lodash',
+    'lodash/get',
+    'lodash/get.js',
+    '@stoplight/path',
+    '@stoplight/spectral-core',
+    '@stoplight/spectral-core/dist/file.js',
+  ])('given valid %s package import, should return true', input => {
+    expect(isPackageImport(input)).toBe(true);
+  });
+
+  it.each(['', 'spectral:oas', '/nimma/legacy', 'path', 'https://cdn.skypack.dev/@stoplight/spectral-core'])(
+    'given invalid %s import, should return false',
+    input => {
+      expect(isPackageImport(input)).toBe(false);
+    },
+  );
+});

packages/ruleset-migrator/src/utils/isKnownNpmRegistry.ts

@@ -0,0 +1,16 @@
+import { parse } from '@stoplight/path';
+
+const KNOWN_PROVIDERS = ['unpkg.com', 'cdn.skypack.dev'];
+
+export function isKnownNpmRegistry(uri: string): boolean {
+  const { protocol, origin } = parse(uri);
+  if (origin === null) {
+    return false;
+  }
+
+  if (protocol !== 'http' && protocol !== 'https') {
+    return false;
+  }
+
+  return KNOWN_PROVIDERS.includes(origin);
+}

packages/ruleset-migrator/src/utils/isPackageImport.ts

@@ -0,0 +1,12 @@
+import validate from 'validate-npm-package-name';
+
+const isValidPackageName = (packageName: string): boolean => validate(packageName).validForNewPackages;
+
+export const isPackageImport = (packageName: string): boolean => {
+  const fragments = packageName.split('/');
+  if (packageName.startsWith('@') && fragments.length >= 2) {
+    fragments.splice(0, 2, `${fragments[0]}/${fragments[1]}`);
+  }
+
+  return fragments.every(isValidPackageName);
+};

yarn.lock

@@ -2148,6 +2148,7 @@ __metadata:
     prettier: ^2.4.1
     reserved: 0.1.2
     tslib: ^2.3.1
+    validate-npm-package-name: 3.0.0
   languageName: unknown
   linkType: soft