diff --git a/src/commands/compare/classes.test.ts b/src/commands/compare/classes.test.ts index d9c5899e..540607f9 100644 --- a/src/commands/compare/classes.test.ts +++ b/src/commands/compare/classes.test.ts @@ -431,8 +431,10 @@ describe('Compare classes', () => { `; const current = ` export declare class Foo { - // 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 { methodA(col: number): number; diff --git a/src/commands/compare/compare.ts b/src/commands/compare/compare.ts index 6f803e11..400e1e0f 100644 --- a/src/commands/compare/compare.ts +++ b/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) { diff --git a/src/commands/compare/functions.test.ts b/src/commands/compare/functions.test.ts index c5110cc4..619904f5 100644 --- a/src/commands/compare/functions.test.ts +++ b/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 a compatible version should not 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); + }); + }); });