fix(core): fix json diff and implicitJsonChanges

Author: FrozenPandaz

packages/workspace/src/core/affected-project-graph/locators/implicit-json-changes.ts

@@ -65,5 +65,5 @@ function getTouchedProjects(path: string[], implicitDependencyConfig: any) {
       break;
     }
   }
-  return found ? curr : [];
+  return found && Array.isArray(curr) ? curr : [];
 }

packages/workspace/src/core/affected-project-graph/locators/npm-packages.spec.ts

@@ -90,6 +90,37 @@ describe('getTouchedNpmPackages', () => {
     expect(result).toEqual(['proj1', 'proj2']);
   });
 
+  it('should handle package addition', () => {
+    const result = getTouchedNpmPackages(
+      [
+        {
+          file: 'package.json',
+          mtime: 0,
+          ext: '.json',
+          getChanges: () => [
+            {
+              type: DiffType.Added,
+              path: ['dependencies', 'awesome-nrwl'],
+              value: {
+                lhs: undefined,
+                rhs: '0.0.1'
+              }
+            }
+          ]
+        }
+      ],
+      workspaceJson,
+      nxJson,
+      {
+        dependencies: {
+          'happy-nrwl': '0.0.2',
+          'awesome-nrwl': '0.0.1'
+        }
+      }
+    );
+    expect(result).toEqual(['awesome-nrwl']);
+  });
+
   it('should handle whole file changes', () => {
     const result = getTouchedNpmPackages(
       [

packages/workspace/src/core/affected-project-graph/locators/npm-packages.ts

@@ -14,7 +14,8 @@ export const getTouchedNpmPackages: TouchedProjectLocator<
   for (const c of changes) {
     if (
       isJsonChange(c) &&
-      (c.path[0] === 'dependencies' || c.path[0] === 'devDependencies')
+      (c.path[0] === 'dependencies' || c.path[0] === 'devDependencies') &&
+      c.path.length === 2
     ) {
       // A package was deleted so mark all workspace projects as touched.
       if (c.type === DiffType.Deleted) {

packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.spec.ts

@@ -95,6 +95,16 @@ describe('getTouchedProjectsInNxJson', () => {
           ext: '.json',
           mtime: 0,
           getChanges: () => [
+            {
+              type: DiffType.Added,
+              path: ['projects', 'proj1'],
+              value: {
+                lhs: undefined,
+                rhs: {
+                  tags: []
+                }
+              }
+            },
             {
               type: DiffType.Added,
               path: ['projects', 'proj1', 'tags'],
@@ -122,7 +132,7 @@ describe('getTouchedProjectsInNxJson', () => {
     expect(result).toEqual(['proj1']);
   });
 
-  it('should not return projects removed in nx.json', () => {
+  it('should return all projects when a project is removed', () => {
     const result = getTouchedProjectsInNxJson(
       [
         {
@@ -132,9 +142,11 @@ describe('getTouchedProjectsInNxJson', () => {
           getChanges: () => [
             {
               type: DiffType.Deleted,
-              path: ['projects', 'proj3', 'tags'],
+              path: ['projects', 'proj3'],
               value: {
-                lhs: [],
+                lhs: {
+                  tags: []
+                },
                 rhs: undefined
               }
             }
@@ -165,6 +177,24 @@ describe('getTouchedProjectsInNxJson', () => {
           ext: '.json',
           mtime: 0,
           getChanges: () => [
+            {
+              type: DiffType.Modified,
+              path: ['projects', 'proj1'],
+              value: {
+                lhs: { tags: ['scope:feat'] },
+                rhs: {
+                  tags: ['scope:shared']
+                }
+              }
+            },
+            {
+              type: DiffType.Modified,
+              path: ['projects', 'proj1', 'tags'],
+              value: {
+                lhs: ['scope:feat'],
+                rhs: ['scope:shared']
+              }
+            },
             {
               type: DiffType.Modified,
               path: ['projects', 'proj1', 'tags', '0'],

packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.ts

@@ -33,13 +33,21 @@ export const getTouchedProjectsInNxJson: TouchedProjectLocator<
       continue;
     }
 
-    if (nxJson.projects[change.path[1]]) {
-      touched.push(change.path[1]);
-    } else {
-      // The project was deleted so affect all projects
-      touched.push(...Object.keys(nxJson.projects));
-      // Break out of the loop after all projects have been added.
-      break;
+    // Only look for changes that are changes to the whole project definition
+    if (change.path.length !== 2) {
+      continue;
+    }
+
+    switch (change.type) {
+      case DiffType.Deleted: {
+        // We are not sure which projects used to depend on a deleted project
+        // so return all projects to be safe
+        return Object.keys(nxJson.projects);
+      }
+      default: {
+        // Add the project name
+        touched.push(change.path[1]);
+      }
     }
   }
   return touched;

packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.spec.ts

@@ -95,10 +95,21 @@ describe('getTouchedProjectsInWorkspaceJson', () => {
           getChanges: () => [
             {
               type: DiffType.Added,
-              path: ['projects', 'proj1', 'tags'],
+              path: ['projects', 'proj1'],
               value: {
                 lhs: undefined,
-                rhs: []
+                rhs: {
+                  root: 'proj1'
+                }
+              }
+            },
+
+            {
+              type: DiffType.Added,
+              path: ['projects', 'proj1', 'root'],
+              value: {
+                lhs: undefined,
+                rhs: 'proj1'
               }
             }
           ]
@@ -125,9 +136,11 @@ describe('getTouchedProjectsInWorkspaceJson', () => {
           getChanges: () => [
             {
               type: DiffType.Deleted,
-              path: ['projects', 'proj3', 'root'],
+              path: ['projects', 'proj3'],
               value: {
-                lhs: 'proj3',
+                lhs: {
+                  root: 'proj3'
+                },
                 rhs: undefined
               }
             }
@@ -156,6 +169,18 @@ describe('getTouchedProjectsInWorkspaceJson', () => {
           ext: '.json',
           mtime: 0,
           getChanges: () => [
+            {
+              type: DiffType.Modified,
+              path: ['projects', 'proj1'],
+              value: {
+                lhs: {
+                  root: 'proj3'
+                },
+                rhs: {
+                  root: 'proj1'
+                }
+              }
+            },
             {
               type: DiffType.Modified,
               path: ['projects', 'proj1', 'root'],

packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.ts

@@ -3,7 +3,7 @@ import {
   WholeFileChange,
   workspaceFileName
 } from '../../file-utils';
-import { isJsonChange, JsonChange } from '../../../utils/json-diff';
+import { DiffType, isJsonChange, JsonChange } from '../../../utils/json-diff';
 import { TouchedProjectLocator } from '../affected-project-graph-models';
 
 export const getTouchedProjectsInWorkspaceJson: TouchedProjectLocator<
@@ -39,13 +39,21 @@ export const getTouchedProjectsInWorkspaceJson: TouchedProjectLocator<
       continue;
     }
 
-    if (workspaceJson.projects[change.path[1]]) {
-      touched.push(change.path[1]);
-    } else {
-      // The project was deleted so affect all projects
-      touched.push(...Object.keys(workspaceJson.projects));
-      // Break out of the loop after all projects have been added.
-      break;
+    // Only look for changes that are changes to the whole project definition
+    if (change.path.length !== 2) {
+      continue;
+    }
+
+    switch (change.type) {
+      case DiffType.Deleted: {
+        // We are not sure which projects used to depend on a deleted project
+        // so return all projects to be safe
+        return Object.keys(workspaceJson.projects);
+      }
+      default: {
+        // Add the project name
+        touched.push(change.path[1]);
+      }
     }
   }
   return touched;

packages/workspace/src/utils/json-diff.spec.ts

@@ -1,14 +1,44 @@
 import { jsonDiff, DiffType } from './json-diff';
 
 describe('jsonDiff', () => {
-  it('should return deep diffs of two JSON objects', () => {
+  it('should return deep diffs of two JSON objects (including parents of children changes)', () => {
     const result = jsonDiff(
       { x: 1, a: { b: { c: 1 } } },
       { y: 2, a: { b: { c: 2, d: 2 } } }
     );
 
     expect(result).toEqual(
       expect.arrayContaining([
+        {
+          type: DiffType.Modified,
+          path: ['a'],
+          value: {
+            lhs: {
+              b: {
+                c: 1
+              }
+            },
+            rhs: {
+              b: {
+                c: 2,
+                d: 2
+              }
+            }
+          }
+        },
+        {
+          type: DiffType.Modified,
+          path: ['a', 'b'],
+          value: {
+            lhs: {
+              c: 1
+            },
+            rhs: {
+              c: 2,
+              d: 2
+            }
+          }
+        },
         {
           type: DiffType.Modified,
           path: ['a', 'b', 'c'],
@@ -33,6 +63,83 @@ describe('jsonDiff', () => {
     );
   });
 
+  it('should have diffs for objects as well', () => {
+    const result = jsonDiff(
+      {
+        a: { b: 0 }
+      },
+
+      {
+        a: { b: 1 }
+      }
+    );
+    expect(result).toContainEqual({
+      type: DiffType.Modified,
+      path: ['a'],
+      value: {
+        lhs: {
+          b: 0
+        },
+        rhs: {
+          b: 1
+        }
+      }
+    });
+    expect(result).toContainEqual({
+      type: DiffType.Modified,
+      path: ['a', 'b'],
+      value: {
+        lhs: 0,
+        rhs: 1
+      }
+    });
+
+    const result2 = jsonDiff(
+      {},
+
+      {
+        a: {}
+      }
+    );
+    expect(result2).toContainEqual({
+      type: DiffType.Added,
+      path: ['a'],
+      value: { lhs: undefined, rhs: {} }
+    });
+  });
+
+  it('should work for added array items', () => {
+    const result = jsonDiff(
+      {
+        rules: ['rule1']
+      },
+      {
+        rules: ['rule1', 'rule2']
+      }
+    );
+
+    expect(result).toEqual(
+      expect.arrayContaining([
+        {
+          type: DiffType.Modified,
+          path: ['rules'],
+          value: {
+            lhs: ['rule1'],
+            rhs: ['rule1', 'rule2']
+          }
+        },
+        {
+          type: DiffType.Added,
+          path: ['rules', '1'],
+          value: {
+            lhs: undefined,
+            rhs: 'rule2'
+          }
+        }
+      ])
+    );
+  });
+
   it('should work well for package.json', () => {
     const result = jsonDiff(
       {

packages/workspace/src/utils/json-diff.ts

@@ -29,9 +29,6 @@ export function jsonDiff(lhs: any, rhs: any): JsonChange[] {
 
   walkJsonTree(lhs, [], (path, lhsValue) => {
     seenInLhs.add(hashArray(path));
-    if (typeof lhsValue === 'object') {
-      return true;
-    }
     const rhsValue = getJsonValue(path, rhs);
     if (rhsValue === undefined) {
       result.push({
@@ -42,7 +39,7 @@ export function jsonDiff(lhs: any, rhs: any): JsonChange[] {
           rhs: undefined
         }
       });
-    } else if (lhsValue !== rhsValue) {
+    } else if (!deepEquals(lhsValue, rhsValue)) {
       result.push({
         type: DiffType.Modified,
         path,
@@ -52,13 +49,10 @@ export function jsonDiff(lhs: any, rhs: any): JsonChange[] {
         }
       });
     }
-    return false;
+    return typeof lhsValue === 'object';
   });
 
   walkJsonTree(rhs, [], (path, rhsValue) => {
-    if (typeof rhsValue === 'object') {
-      return true;
-    }
     const addedInRhs = !seenInLhs.has(hashArray(path));
     if (addedInRhs) {
       result.push({
@@ -71,6 +65,7 @@ export function jsonDiff(lhs: any, rhs: any): JsonChange[] {
       });
       return false;
     }
+    return typeof rhsValue === 'object';
   });
 
   return result;
@@ -85,7 +80,6 @@ export function walkJsonTree(
   if (!json || typeof json !== 'object') {
     return;
   }
-
   Object.keys(json).forEach(key => {
     const path = currPath.concat([key]);
     const shouldContinue = visitor(path, json[key]);
@@ -109,3 +103,32 @@ function getJsonValue(path: string[], json: any): void | any {
   }
   return curr;
 }
+
+function deepEquals(a: any, b: any): boolean {
+  if (a === b) {
+    return true;
+  }
+
+  // Values do not need to be checked for deep equality and the above is false
+  if (
+    // Values are different types
+    typeof a !== typeof b ||
+    // Values are the same type but not an object or array
+    (typeof a !== 'object' && !Array.isArray(a)) ||
+    // Objects are the same type, objects or arrays, but do not have the same number of keys
+    Object.keys(a).length !== Object.keys(b).length
+  ) {
+    return false;
+  }
+
+  // Values need to be checked for deep equality
+  return Object.entries(a).reduce((equal, [key, aValue]) => {
+    // Skip other keys if it is already not equal.
+    if (!equal) {
+      return equal;
+    }
+
+    // Traverse the object
+    return deepEquals(aValue, b[key]);
+  }, true);
+}