fix(@schematics/angular): only issue a warning for addDependency existing specifier

clydin · dgp1130 · commit b8bf3b480bef · 2022-07-13T11:58:37.000-07:00
When using the `addDependency` helper rule, a package specifier mismatch will
now result in a warning instead of an error. This prevents the potential
for breaking changes in schematics that previously allowed the behavior.
The warning will also display the existing and new specifiers to better inform
the schematic user of the outcome of the schematic.
diff --git a/packages/schematics/angular/utility/dependency.ts b/packages/schematics/angular/utility/dependency.ts
@@ -103,13 +103,23 @@ export function addDependency(
     if (!dependencySection) {
       // Section is not present. The dependency can be added to a new object literal for the section.
       manifest[type] = { [name]: specifier };
-    } else if (dependencySection[name] === specifier) {
-      // Already present with same specifier
-      return;
-    } else if (dependencySection[name]) {
-      // Already present but different specifier
-      throw new Error(`Package dependency "${name}" already exists with a different specifier.`);
     } else {
+      const existingSpecifier = dependencySection[name];
+
+      if (existingSpecifier === specifier) {
+        // Already present with same specifier
+        return;
+      }
+
+      if (existingSpecifier) {
+        // Already present but different specifier
+        // This warning may become an error in the future
+        context.logger.warn(
+          `Package dependency "${name}" already exists with a different specifier. ` +
+            `"${existingSpecifier}" will be replaced with "${specifier}".`,
+        );
+      }
+
       // Add new dependency in alphabetical order
       const entries = Object.entries(dependencySection);
       entries.push([name, specifier]);
diff --git a/packages/schematics/angular/utility/dependency_spec.ts b/packages/schematics/angular/utility/dependency_spec.ts
@@ -17,17 +17,31 @@ import {
 } from '@angular-devkit/schematics';
 import { DependencyType, InstallBehavior, addDependency } from './dependency';
 
-async function testRule(rule: Rule, tree: Tree): Promise<TaskConfigurationGenerator[]> {
+interface LogEntry {
+  type: 'warn';
+  message: string;
+}
+
+async function testRule(
+  rule: Rule,
+  tree: Tree,
+): Promise<{ tasks: TaskConfigurationGenerator[]; logs: LogEntry[] }> {
   const tasks: TaskConfigurationGenerator[] = [];
+  const logs: LogEntry[] = [];
   const context = {
     addTask(task: TaskConfigurationGenerator) {
       tasks.push(task);
     },
+    logger: {
+      warn(message: string): void {
+        logs.push({ type: 'warn', message });
+      },
+    },
   };
 
   await callRule(rule, tree, context as unknown as SchematicContext).toPromise();
 
-  return tasks;
+  return { tasks, logs };
 }
 
 describe('addDependency', () => {
@@ -49,7 +63,7 @@ describe('addDependency', () => {
     });
   });
 
-  it('throws if a package is already present with a different specifier', async () => {
+  it('warns if a package is already present with a different specifier', async () => {
     const tree = new EmptyTree();
     tree.create(
       '/package.json',
@@ -60,9 +74,14 @@ describe('addDependency', () => {
 
     const rule = addDependency('@angular/core', '^14.0.0');
 
-    await expectAsync(testRule(rule, tree)).toBeRejectedWithError(
-      undefined,
-      'Package dependency "@angular/core" already exists with a different specifier.',
+    const { logs } = await testRule(rule, tree);
+    expect(logs).toContain(
+      jasmine.objectContaining({
+        type: 'warn',
+        message:
+          'Package dependency "@angular/core" already exists with a different specifier. ' +
+          '"^13.0.0" will be replaced with "^14.0.0".',
+      }),
     );
   });
 
@@ -164,7 +183,7 @@ describe('addDependency', () => {
 
     const rule = addDependency('@angular/core', '^14.0.0');
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -182,7 +201,7 @@ describe('addDependency', () => {
       packageJsonPath: '/abc/package.json',
     });
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -201,7 +220,7 @@ describe('addDependency', () => {
       install: InstallBehavior.Auto,
     });
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -220,7 +239,7 @@ describe('addDependency', () => {
       install: InstallBehavior.Always,
     });
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -239,7 +258,7 @@ describe('addDependency', () => {
       install: InstallBehavior.None,
     });
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks).toEqual([]);
   });
@@ -255,7 +274,7 @@ describe('addDependency', () => {
 
     const rule = addDependency('@angular/core', '^14.0.0');
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks).toEqual([]);
   });
@@ -269,7 +288,7 @@ describe('addDependency', () => {
       addDependency('@angular/common', '^14.0.0'),
     ]);
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -288,7 +307,7 @@ describe('addDependency', () => {
       addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.Auto }),
     ]);
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -307,7 +326,7 @@ describe('addDependency', () => {
       addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.None }),
     ]);
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -326,7 +345,7 @@ describe('addDependency', () => {
       addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.Auto }),
     ]);
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -345,7 +364,7 @@ describe('addDependency', () => {
       addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.Always }),
     ]);
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
@@ -369,7 +388,7 @@ describe('addDependency', () => {
       addDependency('@angular/common', '^14.0.0', { packageJsonPath: '/abc/package.json' }),
     ]);
 
-    const tasks = await testRule(rule, tree);
+    const { tasks } = await testRule(rule, tree);
 
     expect(tasks.map((task) => task.toConfiguration())).toEqual([
       {
