Improved handling of dependency versions #635

smapiot · Oct 18, 2023 · b29c6a2 · b29c6a2
1 parent 6d081fe
commit b29c6a2
Show file tree

Hide file tree

Showing 8 changed files with 933 additions and 539 deletions.
diff --git a/src/tooling/piral-cli-webpack5/package.json b/src/tooling/piral-cli-webpack5/package.json
@@ -53,9 +53,9 @@
     "strip-ansi": "^6.0.0"
   },
   "dependencies": {
-    "@babel/core": "^7.21.8",
-    "@babel/preset-env": "^7.21.5",
-    "@babel/preset-react": "^7.18.6",
+    "@babel/core": "^7.23.2",
+    "@babel/preset-env": "^7.23.2",
+    "@babel/preset-react": "^7.22.15",
     "ansi-html-community": "0.0.8",
     "babel-loader": "^9.1.3",
     "cheerio": "1.0.0-rc.12",

diff --git a/src/tooling/piral-cli/src/common/importmap.test.ts b/src/tooling/piral-cli/src/common/importmap.test.ts
@@ -61,28 +61,20 @@ jest.mock('./io', () => ({
 
 describe('Importmap', () => {
   it('reads empty one from package.json', async () => {
-    const deps = await readImportmap(
-      '/data',
-      {
-        importmap: {},
-      },
-      false,
-    );
+    const deps = await readImportmap('/data', {
+      importmap: {},
+    });
     expect(deps).toEqual([]);
   });
 
   it('reads fully qualified local dependency', async () => {
-    const deps = await readImportmap(
-      '/data',
-      {
-        importmap: {
-          imports: {
-            '[email protected]': 'foo',
-          },
+    const deps = await readImportmap('/data', {
+      importmap: {
+        imports: {
+          '[email protected]': 'foo',
         },
       },
-      false,
-    );
+    });
     expect(deps).toEqual([
       {
         alias: undefined,
@@ -98,17 +90,13 @@ describe('Importmap', () => {
   });
 
   it('reads fully qualified local dependency with implied exact version', async () => {
-    const deps = await readImportmap(
-      '/data',
-      {
-        importmap: {
-          imports: {
-            foo: 'foo',
-          },
+    const deps = await readImportmap('/data', {
+      importmap: {
+        imports: {
+          foo: 'foo',
         },
       },
-      false,
-    );
+    });
     expect(deps).toEqual([
       {
         alias: undefined,
@@ -133,7 +121,6 @@ describe('Importmap', () => {
           },
         },
       },
-      false,
       'match-major',
     );
     expect(deps).toEqual([
@@ -160,7 +147,6 @@ describe('Importmap', () => {
           },
         },
       },
-      false,
       'any-patch',
     );
     expect(deps).toEqual([
@@ -187,7 +173,6 @@ describe('Importmap', () => {
           },
         },
       },
-      false,
       'all',
     );
     expect(deps).toEqual([
@@ -206,47 +191,35 @@ describe('Importmap', () => {
 
   it('fails when the local version does not match the desired version', async () => {
     await expect(
-      readImportmap(
-        '/data',
-        {
-          importmap: {
-            imports: {
-              'bar@^2.0.0': '',
-            },
+      readImportmap('/data', {
+        importmap: {
+          imports: {
+            'bar@^2.0.0': '',
           },
         },
-        false,
-      ),
+      }),
     ).rejects.toThrow();
   });
 
   it('fails when the explicitly shared package is not installed', async () => {
     await expect(
-      readImportmap(
-        '/data',
-        {
-          importmap: {
-            imports: {
-              'qxz': '',
-            },
+      readImportmap('/data', {
+        importmap: {
+          imports: {
+            qxz: '',
           },
         },
-        false,
-      ),
+      }),
     ).rejects.toThrow();
   });
 
   it('accepts an importmap with valid resolvable inherited deps', async () => {
-    const deps = await readImportmap(
-      '/data',
-      {
-        importmap: {
-          imports: {},
-          inherit: ['app1'],
-        },
+    const deps = await readImportmap('/data', {
+      importmap: {
+        imports: {},
+        inherit: ['app1'],
       },
-      false,
-    );
+    });
     expect(deps).toEqual([
       {
         alias: undefined,
@@ -263,16 +236,12 @@ describe('Importmap', () => {
   });
 
   it('accepts an importmap with valid but unresolvable inherited deps', async () => {
-    const deps = await readImportmap(
-      '/data',
-      {
-        importmap: {
-          imports: {},
-          inherit: ['app2'],
-        },
+    const deps = await readImportmap('/data', {
+      importmap: {
+        imports: {},
+        inherit: ['app2'],
       },
-      false,
-    );
+    });
     expect(deps).toEqual([]);
   });
 });
diff --git a/src/tooling/piral-cli/src/common/importmap.ts b/src/tooling/piral-cli/src/common/importmap.ts
@@ -42,16 +42,28 @@ function getMatchMajor(version: string) {
   return `^${major}.0.0`;
 }
 
+function makeAssetName(id: string) {
+  return (id.startsWith('@') ? id.substring(1) : id).replace(/[\/\.]/g, '-').replace(/(\-)+/, '-');
+}
+
 function getDependencyDetails(
   depName: string,
+  availableSpecs: Record<string, string>,
 ): [assetName: string, identifier: string, versionSpec: string, isAsync: boolean] {
   const name = depName.replace(/\?+$/, '');
   const isAsync = depName.endsWith('?');
   const sep = name.indexOf('@', 1);
-  const version = sep > 0 ? name.substring(sep + 1) : '';
-  const id = sep > 0 ? name.substring(0, sep) : name;
-  const assetName = (id.startsWith('@') ? id.substring(1) : id).replace(/[\/\.]/g, '-').replace(/(\-)+/, '-');
-  return [assetName, id, version, isAsync];
+
+  if (sep > 0) {
+    const id = name.substring(0, sep);
+    const version = name.substring(sep + 1);
+    const assetName = makeAssetName(id);
+    return [assetName, id, version, isAsync];
+  } else {
+    const version = availableSpecs[name] || '';
+    const assetName = makeAssetName(name);
+    return [assetName, name, version, isAsync];
+  }
 }
 
 async function getLocalDependencyVersion(
@@ -70,7 +82,7 @@ async function getLocalDependencyVersion(
     }
 
     if (!satisfies(details.version, versionSpec)) {
-      fail('importMapVersionSpecNotSatisfied_0025', depName, details.version);
+      fail('importMapVersionSpecNotSatisfied_0025', depName, details.version, versionSpec);
     }
 
     return [details.name, details.version, versionSpec];
@@ -89,20 +101,26 @@ async function getLocalDependencyVersion(
   }
 }
 
-async function getInheritedDependencies(inheritedImport: string, dir: string): Promise<Array<SharedDependency>> {
+async function getInheritedDependencies(
+  inheritedImport: string,
+  dir: string,
+  excludedDependencies: Array<string>,
+): Promise<Array<SharedDependency>> {
   const packageJson = tryResolvePackage(`${inheritedImport}/package.json`, dir);
 
   if (packageJson) {
     const packageDir = dirname(packageJson);
     const packageDetails = await readJson(packageDir, 'package.json');
-    return await readImportmap(packageDir, packageDetails, true, 'exact');
+    return await consumeImportmap(packageDir, packageDetails, true, 'exact', excludedDependencies);
   } else {
     const directImportmap = tryResolvePackage(inheritedImport, dir);
 
     if (directImportmap) {
       const baseDir = dirname(directImportmap);
       const content = await readJson(baseDir, basename(directImportmap));
       return await resolveImportmap(baseDir, content, {
+        availableSpecs: {},
+        excludedDependencies,
         ignoreFailure: true,
         versionBehavior: 'exact',
       });
@@ -113,6 +131,8 @@ async function getInheritedDependencies(inheritedImport: string, dir: string): P
 }
 
 interface ImportmapResolutionOptions {
+  availableSpecs: Record<string, string>;
+  excludedDependencies: Array<string>;
   versionBehavior: ImportmapVersions;
   ignoreFailure: boolean;
 }
@@ -126,6 +146,7 @@ async function resolveImportmap(
   const sharedImports = importmap?.imports;
   const inheritedImports = importmap?.inherit;
   const excludedImports = importmap?.exclude;
+
   const onUnresolved = (entry: string) => {
     if (options.ignoreFailure) {
       log('skipUnresolvedDependency_0054', entry);
@@ -137,9 +158,11 @@ async function resolveImportmap(
   if (typeof sharedImports === 'object' && sharedImports) {
     for (const depName of Object.keys(sharedImports)) {
       const url = sharedImports[depName];
-      const [assetName, identifier, versionSpec, isAsync] = getDependencyDetails(depName);
+      const [assetName, identifier, versionSpec, isAsync] = getDependencyDetails(depName, options.availableSpecs);
 
-      if (typeof url !== 'string') {
+      if (options.excludedDependencies.includes(identifier)) {
+        continue;
+      } else if (typeof url !== 'string') {
         log('generalInfo_0000', `The value of "${depName}" in the importmap is not a string and will be ignored.`);
       } else if (/^https?:\/\//.test(url)) {
         const hash = computeHash(url).substring(0, 7);
@@ -255,14 +278,14 @@ async function resolveImportmap(
   }
 
   if (Array.isArray(inheritedImports)) {
+    const excluded = Array.isArray(excludedImports)
+      ? [...options.excludedDependencies, ...excludedImports]
+      : options.excludedDependencies;
+
     for (const inheritedImport of inheritedImports) {
-      const otherDependencies = await getInheritedDependencies(inheritedImport, dir);
+      const otherDependencies = await getInheritedDependencies(inheritedImport, dir, excluded);
 
       for (const dependency of otherDependencies) {
-        if (Array.isArray(excludedImports) && excludedImports.includes(dependency.name)) {
-          continue;
-        }
-
         const entry = dependencies.find((dep) => dep.name === dependency.name);
 
         if (!entry) {
@@ -279,14 +302,15 @@ async function resolveImportmap(
 
   return dependencies;
 }
-
-export async function readImportmap(
+async function consumeImportmap(
   dir: string,
   packageDetails: any,
-  inherited = false,
-  versionBehavior: ImportmapVersions = 'exact',
+  inherited: boolean,
+  versionBehavior: ImportmapVersions,
+  excludedDependencies: Array<string>,
 ): Promise<Array<SharedDependency>> {
   const importmap = packageDetails.importmap;
+  const availableSpecs = inherited ? packageDetails.devDependencies : {};
 
   if (typeof importmap === 'string') {
     const notFound = {};
@@ -298,7 +322,9 @@ export async function readImportmap(
 
     const baseDir = dirname(resolve(dir, importmap));
     return await resolveImportmap(baseDir, content, {
+      availableSpecs,
       ignoreFailure: inherited,
+      excludedDependencies,
       versionBehavior,
     });
   } else if (typeof importmap === 'undefined' && inherited) {
@@ -318,7 +344,17 @@ export async function readImportmap(
   }
 
   return await resolveImportmap(dir, importmap, {
+    availableSpecs,
+    excludedDependencies,
     ignoreFailure: inherited,
     versionBehavior,
   });
 }
+
+export function readImportmap(
+  dir: string,
+  packageDetails: any,
+  versionBehavior: ImportmapVersions = 'exact',
+): Promise<Array<SharedDependency>> {
+  return consumeImportmap(dir, packageDetails, false, versionBehavior, []);
+}
diff --git a/src/tooling/piral-cli/src/common/package.ts b/src/tooling/piral-cli/src/common/package.ts
@@ -819,7 +819,7 @@ export async function retrievePiletData(target: string, app?: string) {
     });
   }
 
-  const importmap = await readImportmap(root, piletPackage, undefined, piletDefinition?.importmapVersions);
+  const importmap = await readImportmap(root, piletPackage, piletDefinition?.importmapVersions);
 
   return {
     dependencies: piletPackage.dependencies || {},

diff --git a/src/tooling/piral-cli/src/messages.ts b/src/tooling/piral-cli/src/messages.ts
@@ -493,8 +493,16 @@ export function packageVersionInvalid_0024(version: string): QuickMessage {
  * The best way, however, is to look at the used version and adjust the specifier to be correct again.
  * Alternatively, change the used version to satisfy the constraint again.
  */
-export function importMapVersionSpecNotSatisfied_0025(depName: string, version: string): QuickMessage {
-  return [LogLevels.error, '0025', `The dependency "${depName}" in only available in version "${version}".`];
+export function importMapVersionSpecNotSatisfied_0025(
+  depName: string,
+  availableVersion: string,
+  specVersion: string,
+): QuickMessage {
+  return [
+    LogLevels.error,
+    '0025',
+    `The dependency "${depName}" is locally installed in version "${availableVersion}", but specified as "${specVersion}".`,
+  ];
 }
 
 /**
@@ -1313,8 +1321,8 @@ export function cannotResolveDependency_0053(names: Array<string>, rootDir: stri
  * When a pilet is built all the inherited (i.e., centrally shared) dependencies will be resolved. In case
  * you did not install one of these dependencies a short info will be shown. This acts as a reminder that
  * you could install more dependencies - without any runtime cost.
- * 
- * Note that even though shared dependencies are available at runtime in any case they will might be 
+ *
+ * Note that even though shared dependencies are available at runtime in any case they will might be
  * for building your pilet. Therefore, if you plan to use shared dependencies please install them in your
  * pilet's repository.
  *

diff --git a/src/tooling/piral-cli/src/rules/pilet-has-externals-as-peers.ts b/src/tooling/piral-cli/src/rules/pilet-has-externals-as-peers.ts
@@ -18,6 +18,8 @@ Received: Missing "${missingNames.join('", "')}".
 
 /**
  * Checks that "externals" dependencies have been specified in "peerDependencies".
+ * This is legacy and only used if no importmap has been specified.
+ * Importmap inherited dependencies are auto-checked.
  */
 export default async function (context: PiletRuleContext, options: Options = 'ignore') {
   // only check if options are not set to ignore and if importmap feature is not used