Fix bug with storing element declaration references (#1372)

Author: justinfagnani

packages/catalog-server/src/lib/firestore/firestore-repository.ts

@@ -342,14 +342,17 @@ export class FirestoreRepository implements Repository {
         className: c.declaration.name,
         customElementExport: referenceString(
           packageName,
-          c.module,
+          c.module.path,
           c.export.name
         ),
-        declaration: referenceString(packageName, c.module, c.declaration.name),
+        declaration: referenceString(
+          packageName,
+          c.declarationReference.module ?? c.module.path,
+          c.declaration.name
+        ),
         searchTerms,
       });
     }
-
     await batch.commit();
   }
 

packages/catalog-server/src/test/lib/catalog_test.ts

@@ -18,18 +18,27 @@ import {
 } from '@webcomponents/catalog-api/lib/schema.js';
 import {fileURLToPath} from 'url';
 import {Temporal} from '@js-temporal/polyfill';
+import {
+  getModule,
+  parseReferenceString,
+  resolveReference,
+} from '@webcomponents/custom-elements-manifest-tools';
 
 const test = suite('Catalog tests');
 
 const testPackage1Path = fileURLToPath(
   new URL('../test-packages/test-1/', import.meta.url)
 );
+const testPackage2Path = fileURLToPath(
+  new URL('../test-packages/test-2/', import.meta.url)
+);
 
 // A set of import, fetch, search tests that use the same data
 const TEST_SEQUENCE_ONE = 'test-data-1';
 
 // Other tests than can run independently
 const TEST_SEQUENCE_TWO = 'test-data-2';
+const TEST_SEQUENCE_THREE = 'test-data-3';
 
 test('Imports a package with no problems', async () => {
   const packageName = 'test-1';
@@ -223,4 +232,57 @@ test('Updates a package', async () => {
   assert.equal(isReadablePackageVersion(result), true);
 });
 
+test('Imports a package with separate define and implementation modules', async () => {
+  const packageName = 'test-2';
+  const version = '1.0.0';
+  const files = new LocalFsPackageFiles({
+    path: testPackage2Path,
+    packageName,
+    publishedVersions: ['1.0.0'],
+    distTags: {
+      latest: '1.0.0',
+    },
+  });
+  const repository = new FirestoreRepository(TEST_SEQUENCE_THREE);
+  const catalog = new Catalog({files, repository});
+  await catalog.importPackage(packageName);
+
+  const packageVersion = (await catalog.getPackageVersion(
+    packageName,
+    version
+  )) as ReadablePackageVersion;
+
+  const customElements = await catalog.getCustomElements(
+    packageName,
+    version,
+    undefined
+  );
+
+  assert.ok(packageVersion.customElementsManifest);
+  const manifest =
+    packageVersion.customElementsManifest !== undefined &&
+    JSON.parse(packageVersion.customElementsManifest);
+
+  assert.ok(customElements);
+  const fooElement = customElements.find((c) => c.tagName === 'foo-element');
+  assert.ok(fooElement);
+  assert.ok(fooElement.declaration);
+
+  const declarationRefString = fooElement.declaration;
+  const declarationRef = parseReferenceString(declarationRefString);
+  assert.ok(declarationRef.module);
+
+  const module = getModule(manifest, declarationRef.module);
+  assert.ok(module);
+
+  const declaration = resolveReference(
+    manifest,
+    module,
+    declarationRef,
+    packageName,
+    ''
+  );
+  assert.ok(declaration);
+});
+
 test.run();

packages/catalog-server/test/test-packages/test-2/1.0.0/custom-elements.json

@@ -0,0 +1,45 @@
+{
+  "schemaVersion": "1.0.0",
+  "description": "A set of cool elements",
+  "modules": [
+    {
+      "kind": "javascript-module",
+      "path": "foo.js",
+      "exports": [
+        {
+          "kind": "custom-element-definition",
+          "name": "foo-element",
+          "declaration": {
+            "name": "FooElement",
+            "module": "/foo-impl.js"
+          }
+        }
+      ],
+      "declarations": []
+    },
+    {
+      "kind": "javascript-module",
+      "path": "foo-impl.js",
+      "exports": [
+        {
+          "kind": "js",
+          "name": "FooElement",
+          "declaration": {
+            "name": "FooElement"
+          }
+        }
+      ],
+      "declarations": [
+        {
+          "kind": "class",
+          "customElement": true,
+          "tagName": "foo-element",
+          "name": "FooElement",
+          "superclass": {
+            "name": "HTMLElement"
+          }
+        }
+      ]
+    }
+  ]
+}

packages/catalog-server/test/test-packages/test-2/1.0.0/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "test-2",
+  "version": "1.0.0",
+  "description": "A set of cool elements",
+  "customElements": "custom-elements.json"
+}

packages/custom-elements-manifest-tools/package.json

@@ -26,6 +26,7 @@
         "lib/",
         "test/",
         "!test/test-data/shoelace-2.0.0-beta.83.json",
+        "!test/test-data/lion-button-0.18.1.json",
         "tsconfig.tsbuildinfo"
       ],
       "clean": "if-file-deleted"
@@ -36,7 +37,8 @@
         "build"
       ],
       "files": [
-        "test/test-data/shoelace-2.0.0-beta.83.json"
+        "test/test-data/shoelace-2.0.0-beta.83.json",
+        "test/test-data/lion-button-0.18.1.json"
       ],
       "output": []
     }

packages/custom-elements-manifest-tools/src/lib/get-custom-elements.ts

@@ -9,6 +9,7 @@ import type {
   CustomElementExport,
   Module,
   Package,
+  Reference,
 } from 'custom-elements-manifest/schema.js';
 import {isCustomElementDeclaration} from './predicates.js';
 import {resolveReference} from './resolve-reference.js';
@@ -18,6 +19,7 @@ export type CustomElementInfo = {
   module: Module;
   export: CustomElementExport;
   declaration: CustomElementDeclaration;
+  declarationReference: Reference;
 };
 
 /**
@@ -48,6 +50,7 @@ export const getCustomElements = (
               module: mod,
               export: e,
               declaration: decl,
+              declarationReference: e.declaration,
             });
           } else {
             // This is some kind of manifest error, should we warn?

packages/custom-elements-manifest-tools/src/lib/reference-string.ts

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import type {Module, Reference} from 'custom-elements-manifest/schema.js';
+import type {Reference} from 'custom-elements-manifest/schema.js';
 
 /**
  * Serializes a reference to a string, as used in the GraphQL API of the
@@ -15,10 +15,13 @@ import type {Module, Reference} from 'custom-elements-manifest/schema.js';
  */
 export const referenceString = (
   packageName: string,
-  mod: Module,
+  modulePath: string,
   name: string
 ) => {
-  return `${packageName}/${mod.path}#${name}`;
+  if (modulePath.startsWith('/')) {
+    modulePath = modulePath.substring(1);
+  }
+  return `${packageName}/${modulePath}#${name}`;
 };
 
 /**

packages/custom-elements-manifest-tools/src/test/lib/get-custom-elements_test.ts

@@ -101,4 +101,22 @@ test('Get elements from Shoelace', async () => {
   assert.equal(customElements.length, 54);
 });
 
+test('Get elements from @liton/burron', async () => {
+  const manifestPath = fileURLToPath(
+    new URL('../test-data/lion-button-0.18.1.json', import.meta.url)
+  );
+  const manifestSource = await readFile(manifestPath, 'utf-8');
+  const manifest = JSON.parse(manifestSource);
+  const customElements = getCustomElements(
+    manifest,
+    '@lion/button',
+    '0.18.1'
+  );
+  assert.equal(customElements.length, 3);
+
+  const element = customElements.find((e) => e.export.name === 'lion-button-submit');
+  assert.ok(element);
+  assert.ok(element.declaration);
+});
+
 test.run();

packages/custom-elements-manifest-tools/src/test/lib/resolve-reference_test.ts

@@ -48,6 +48,42 @@ const pkg: Package = {
         },
       ],
     },
+    {
+      kind: 'javascript-module',
+      path: 'x-foo.js',
+      declarations: [],
+      exports: [
+        {
+          kind: 'custom-element-definition',
+          name: 'x-foo',
+          declaration: {
+            name: 'XFoo',
+            module: '/src/components/x-foo.js',
+          },
+        },
+      ],
+    },
+    {
+      kind: 'javascript-module',
+      path: 'src/components/x-foo.js',
+      declarations: [
+        {
+          kind: 'class',
+          name: 'XFoo',
+          customElement: true,
+        },
+      ],
+      exports: [
+        {
+          kind: 'js',
+          name: 'XFoo',
+          declaration: {
+            name: 'XFoo',
+            module: 'src/components/x-foo.js',
+          },
+        },
+      ],
+    },
   ],
 };
 
@@ -120,4 +156,19 @@ test('cross-package references are unsupported', () => {
   assert.equal(result, undefined);
 });
 
+test(`resolves a custom element defintion to it's declaration`, () => {
+  const definitionModule = pkg.modules.find((m) => m.path === 'x-foo.js')!;
+  const elementExport = definitionModule.exports![0];
+  const declarationReference = elementExport!.declaration;
+  const result = resolveReference(
+    pkg,
+    definitionModule,
+    declarationReference,
+    'test-package',
+    '1.0.0'
+  );
+  console.log('result', result);
+  assert.ok(result);
+});
+
 test.run();