Skip to content

Commit

Permalink
Revert "Detect deeper function parameter changes (#361)" (#364)
Browse files Browse the repository at this point in the history 
This reverts commit 80f1ca3.
  • Loading branch information
@academo
academo authored Dec 7, 2023
1 parent 3ab7b9a commit db2c18b
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 182 deletions.
9 changes: 3 additions & 6 deletions src/commands/compare/classes.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,10 +431,8 @@ describe('Compare classes', () => {
`;
const current = `
export declare class Foo<T = any> {
// CHANGED parameter name but should not be reported
// CHANGED
methodA(colName: number): number;
// CHANGED from string to Boolean. Should be reported
methodB(col: Boolean): string;
// NO CHANGE
Expand All @@ -443,8 +441,7 @@ describe('Compare classes', () => {
`;
const comparison = testCompare(prev, current);

expect(Object.keys(comparison.changes)).toEqual(['Foo', 'Foo.methodB']);
expect(Object.keys(comparison.changes).length).toBe(2);
expect(Object.keys(comparison.changes).length).toBe(3); // Foo, Foo.methodA and Foo.methodB changed
expect(Object.keys(comparison.additions).length).toBe(0);
expect(Object.keys(comparison.removals).length).toBe(0);
});
Expand Down Expand Up @@ -488,7 +485,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;
Expand Down
46 changes: 30 additions & 16 deletions src/commands/compare/compare.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,11 @@ 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 prevParamTypeNode = prevDeclaration.parameters[i].type;
const prevParamType = 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]) {
Expand All @@ -159,11 +157,9 @@ export function getFunctionParametersDiff({
};
}

// 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)) {
// Changed parameter at the old position
if (currentDeclaration.parameters[i].getText() !== prevDeclaration.parameters[i].getText()) {
const currentParamSymbol = getSymbolFromParameter(currentDeclaration.parameters[i], current.program);
return {
prev: prevParamSymbol,
prevProgram: prev.program,
Expand All @@ -172,6 +168,26 @@ 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
Expand Down Expand Up @@ -202,15 +218,13 @@ export function hasFunctionChanged(prev: SymbolMeta, current: SymbolMeta) {
return true;
}

const checker = prev.program.getTypeChecker();

const prevDeclarationType = checker.getTypeAtLocation(prevDeclaration);
const currentDeclarationType = checker.getTypeAtLocation(currentDeclaration);
// 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;
}

return !(
checker.isTypeAssignableTo(prevDeclarationType, currentDeclarationType) &&
checker.isTypeAssignableTo(currentDeclarationType, prevDeclarationType)
);
return false;
}

function hasInterfaceChanged(prev: SymbolMeta, current: SymbolMeta) {
Expand Down
160 changes: 0 additions & 160 deletions src/commands/compare/functions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,164 +123,4 @@ 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);
});
});
});

0 comments on commit db2c18b

Please sign in to comment.