Detect deeper function parameter changes

src/commands/compare/classes.test.ts

@@ -431,8 +431,10 @@ describe('Compare classes', () => {
     `;
     const current = `
       export declare class Foo<T = any> {
-        // CHANGED
+        // CHANGED parameter name but should not be reported
         methodA(colName: number): number;
+
+        // CHANGED from string to Boolean. Should be reported
         methodB(col: Boolean): string;
 
         // NO CHANGE
@@ -441,7 +443,8 @@ describe('Compare classes', () => {
     `;
     const comparison = testCompare(prev, current);
 
-    expect(Object.keys(comparison.changes).length).toBe(3); // Foo, Foo.methodA and Foo.methodB changed
+    expect(Object.keys(comparison.changes)).toEqual(['Foo', 'Foo.methodB']);
+    expect(Object.keys(comparison.changes).length).toBe(2);
     expect(Object.keys(comparison.additions).length).toBe(0);
     expect(Object.keys(comparison.removals).length).toBe(0);
   });
@@ -485,7 +488,7 @@ describe('Compare classes', () => {
     expect(Object.keys(comparison.removals).length).toBe(1); // Foo.methodA removed
   });
 
- test('changing the generic should trigger a change', () => {
+  test('changing the generic should trigger a change', () => {
     const prev = `
       export declare class Foo<T = any> {
         methodA(col: number): number;

src/commands/compare/compare.ts

@@ -140,11 +140,13 @@ export function getFunctionParametersDiff({
 }): Change | undefined {
   const prevDeclaration = prev.symbol.valueDeclaration as ts.FunctionDeclaration;
   const currentDeclaration = current.symbol.valueDeclaration as ts.FunctionDeclaration;
+  const checker = prev.program.getTypeChecker();
   // Check previous function parameters
   // (all previous parameters must be present at their previous position)
   for (let i = 0; i < prevDeclaration.parameters.length; i++) {
-    const prevParamType = prevDeclaration.parameters[i].type;
+    // const prevParamTypeNode = prevDeclaration.parameters[i].type;
     const prevParamSymbol = getSymbolFromParameter(prevDeclaration.parameters[i], prev.program);
+    const prevParamType = checker.getTypeAtLocation(prevDeclaration.parameters[i]);
 
     // No parameter at the same position
     if (!currentDeclaration.parameters[i]) {
@@ -157,9 +159,11 @@ export function getFunctionParametersDiff({
       };
     }
 
-    // Changed parameter at the old position
-    if (currentDeclaration.parameters[i].getText() !== prevDeclaration.parameters[i].getText()) {
-      const currentParamSymbol = getSymbolFromParameter(currentDeclaration.parameters[i], current.program);
+    // const currentParamTypeNode = currentDeclaration.parameters[i]?.type || undefined;
+    const currentParamSymbol = getSymbolFromParameter(currentDeclaration.parameters[i], current.program);
+    const currentParamType = checker.getTypeAtLocation(currentDeclaration.parameters[i]);
+
+    if (!checker.isTypeAssignableTo(prevParamType, currentParamType)) {
       return {
         prev: prevParamSymbol,
         prevProgram: prev.program,
@@ -168,26 +172,6 @@ export function getFunctionParametersDiff({
         type: ChangeType.PARAMETER_NAME,
       };
     }
-
-    const currentParamType = currentDeclaration.parameters[i]?.type || undefined;
-    const currentParamSymbol = getSymbolFromParameter(currentDeclaration.parameters[i], current.program);
-
-    // native types (e.g. MouseEvent) don't have declarations
-    if (!currentParamSymbol.declarations || !prevParamSymbol.declarations) {
-      return;
-    }
-
-    if (ts.isTypeReferenceNode(currentParamType) && ts.isTypeReferenceNode(prevParamType)) {
-      if (currentParamSymbol.declarations[0].getText() !== prevParamSymbol.declarations[0].getText()) {
-        return {
-          prev: prevParamSymbol,
-          prevProgram: prev.program,
-          current: currentParamSymbol,
-          currentProgram: current.program,
-          type: ChangeType.PARAMETER_TYPE,
-        };
-      }
-    }
   }
 
   // Check current function parameters
@@ -218,13 +202,15 @@ export function hasFunctionChanged(prev: SymbolMeta, current: SymbolMeta) {
     return true;
   }
 
-  // Check return type signatures -> they must be the same
-  // (It can happen that a function/method does not have a return type defined)
-  if (prevDeclaration.type?.getText() !== currentDeclaration.type?.getText()) {
-    return true;
-  }
+  const checker = prev.program.getTypeChecker();
 
-  return false;
+  const prevDeclarationType = checker.getTypeAtLocation(prevDeclaration);
+  const currentDeclarationType = checker.getTypeAtLocation(currentDeclaration);
+
+  return !(
+    checker.isTypeAssignableTo(prevDeclarationType, currentDeclarationType) &&
+    checker.isTypeAssignableTo(currentDeclarationType, prevDeclarationType)
+  );
 }
 
 function hasInterfaceChanged(prev: SymbolMeta, current: SymbolMeta) {

src/commands/compare/functions.test.ts

@@ -123,4 +123,164 @@ describe('Compare functions', () => {
     expect(Object.keys(comparison.additions).length).toBe(0);
     expect(Object.keys(comparison.removals).length).toBe(0);
   });
+
+  describe('Deeper function parameters', () => {
+    test('Changing an unexported type of a parameter to a compatible version should not trigger a breaking change', () => {
+      const prev = `
+      type TestType = 'a' | 'b';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      // adding to union type is compatible
+      const current = `
+      type TestType = 'a' | 'b' | 'c'; // added 'c'
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(0);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+
+    test('Changing an unexported type of a parameter to an incompatible version should trigger a breaking change', () => {
+      const prev = `
+      type TestType = 'a' | 'b' | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      //removing from union type is not compatible
+      const current = `
+      type TestType = 'a' | 'b'; // removed 'c'
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(1);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+
+    test('Changing an unexported type of a parameter to an compatible version should trigger a breaking change (chained union types)', () => {
+      const prev = `
+      type InnerType = 'a' | 'b';
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      //adding to inner union type is still compatible
+      const current = `
+      type InnerType = 'a' | 'b' | 'x'; // added 'x'
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(0);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+
+    test('Changing an unexported type of a parameter to an incompatible version should trigger a breaking change (chained union types)', () => {
+      const prev = `
+      type InnerType = 'a' | 'b';
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      //removing from union type is not compatible
+      const current = `
+      type InnerType = 'a'; // removed 'b'
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(1);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+
+    test('Changing a deep-nested type of a parameter to an incompatible version should trigger a breaking change', () => {
+      const prev = `
+      interface InnerType {
+        a: string;
+        b: string;
+      }
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      //changing deep nested type is not compatible
+      const current = `
+      interface InnerType {
+        a: string;
+        b: number;
+      }
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(1);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+
+    test('Changing a deep-nested type of a parameter to an incompatible version should trigger a breaking change', () => {
+      const prev = `
+      type DeepInnerType = 'a' | 'b';
+      interface InnerType {
+        a: string;
+        b: DeepInnerType;
+      }
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      //changing deep nested type is not compatible
+      const current = `
+      type DeepInnerType = 'a'; // removed 'b'
+      interface InnerType {
+        a: string;
+        b: DeepInnerType;
+      }
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(1);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+
+    test('Changing a deep-nested type of a parameter to a compatible version should not trigger a breaking change', () => {
+      const prev = `
+      type DeepInnerType = 'a' | 'b';
+      interface InnerType {
+        a: string;
+        b: DeepInnerType;
+      }
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+
+      //adding to deep nested type is still compatible
+      const current = `
+      type DeepInnerType = 'a' | 'b' | 'x'; // added 'x'
+      interface InnerType {
+        a: string;
+        b: DeepInnerType;
+      }
+      type TestType = InnerType | 'c';
+      export function foo(a: string, b: TestType): string {};
+    `;
+      const comparison = testCompare(prev, current);
+
+      expect(Object.keys(comparison.changes).length).toBe(0);
+      expect(Object.keys(comparison.additions).length).toBe(0);
+      expect(Object.keys(comparison.removals).length).toBe(0);
+    });
+  });
 });