fix(ruleset-migrator): resolve npm packages correctly

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__/__fixtures__/extends-variant-9/ruleset.yaml

@@ -0,0 +1,2 @@
+extends:
+  - some-npm-ruleset-i-will-have-to-add-somehow

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

@@ -8,6 +8,7 @@ import * as fetchMock from 'fetch-mock';
 
 import { migrateRuleset } from '..';
 import * as fixtures from './__fixtures__/.cache/index.json';
+import { serveAssets } from '../../../../test-utils/node';
 
 const cwd = '/.tmp/spectral';
 
@@ -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

@@ -40,11 +40,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,13 @@
 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';
 
 export { Scope };
 
@@ -31,7 +32,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 +83,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 +122,21 @@ 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)) {
+      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__/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(['', '/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/isPackageImport.ts

@@ -0,0 +1,12 @@
+import * as 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