From dbf6fd525d8d03cc636e718ec2e5a626ee9a26e0 Mon Sep 17 00:00:00 2001 From: Jason Jean Date: Thu, 5 Mar 2020 20:51:16 -0500 Subject: [PATCH] fix(core): fix json diff and implicitJsonChanges --- .../locators/implicit-json-changes.ts | 2 +- .../locators/npm-packages.spec.ts | 31 +++++ .../locators/npm-packages.ts | 3 +- .../locators/nx-json-changes.spec.ts | 36 +++++- .../locators/nx-json-changes.ts | 22 ++-- .../locators/workspace-json-changes.spec.ts | 33 +++++- .../locators/workspace-json-changes.ts | 24 ++-- .../workspace/src/utils/json-diff.spec.ts | 109 +++++++++++++++++- packages/workspace/src/utils/json-diff.ts | 41 +++++-- 9 files changed, 267 insertions(+), 34 deletions(-) diff --git a/packages/workspace/src/core/affected-project-graph/locators/implicit-json-changes.ts b/packages/workspace/src/core/affected-project-graph/locators/implicit-json-changes.ts index 270d7ae945..7206dd989c 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/implicit-json-changes.ts +++ b/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 : []; } diff --git a/packages/workspace/src/core/affected-project-graph/locators/npm-packages.spec.ts b/packages/workspace/src/core/affected-project-graph/locators/npm-packages.spec.ts index 563bcb60a7..32337351b3 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/npm-packages.spec.ts +++ b/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( [ diff --git a/packages/workspace/src/core/affected-project-graph/locators/npm-packages.ts b/packages/workspace/src/core/affected-project-graph/locators/npm-packages.ts index 345aa1450f..d5b575a543 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/npm-packages.ts +++ b/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) { diff --git a/packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.spec.ts b/packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.spec.ts index 966b5a3c46..3261549a8d 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.spec.ts +++ b/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'], diff --git a/packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.ts b/packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.ts index e0c891b676..068d002685 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/nx-json-changes.ts +++ b/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; diff --git a/packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.spec.ts b/packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.spec.ts index 331d5ca2a0..195568c830 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.spec.ts +++ b/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'], diff --git a/packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.ts b/packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.ts index 7ff6fa2abf..d9c6dfa0d2 100644 --- a/packages/workspace/src/core/affected-project-graph/locators/workspace-json-changes.ts +++ b/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; diff --git a/packages/workspace/src/utils/json-diff.spec.ts b/packages/workspace/src/utils/json-diff.spec.ts index c002c0b919..1ec2dda564 100644 --- a/packages/workspace/src/utils/json-diff.spec.ts +++ b/packages/workspace/src/utils/json-diff.spec.ts @@ -1,7 +1,7 @@ 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 } } } @@ -9,6 +9,36 @@ describe('jsonDiff', () => { 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( { diff --git a/packages/workspace/src/utils/json-diff.ts b/packages/workspace/src/utils/json-diff.ts index 6ff1c38835..127051a748 100644 --- a/packages/workspace/src/utils/json-diff.ts +++ b/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); +}