Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch Basic Detection for RLC #9209

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

wanlwanl
Copy link
Member

@wanlwanl wanlwanl commented Oct 17, 2024

Features

  • General interface breaking change detection, including Routes in RLC
  • General function and function overloads' breaking change detection: . e.g. IsUnexpected
  • Type alias breaking changes detection

Fixes

  • export type A = "X" | "Y" -> export type A ="X" | "y" cannot be detected
  • Except for intersection type, type alias changes cannot be detected
  • Emoved type alias cannot be detected
  • Routes changes cannot be detected
  • Function and function overloads cannot be detected

Notes

  • Corner cases are waiting for review, see loop, therefore, findBreakingReasons function in declaration-diff.ts file may be inaccurate. Except for this, the PR is ready for review

@wanlwanl wanlwanl marked this pull request as ready for review October 17, 2024 04:22
- Added Interface ClusterGetOld
- Added Type Alias typesAdd
- Added function funcAdd
- Added function overload "export function isUnexpected(response: C | E): response is C;"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this might be an internal implementation detail. Not sure we want to generate changelog for this?

- Operation in "Routes" has a new signature for path: "change_para_type"
- Type of parameter a of interface ClustersGet is changed from number to string
- Removed function funcRemove
- Function funcReturnType has a new signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could show what the signature was like before and after? how the changelog would look like?

- Function funcParameterType has a new signature
- Removed function overload 'export function isUnexpected(response: C | D): response is A;'
- Removed Type Alias typesRemove
- Type alias "typesChange" has been changed`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what's the type alias be like before and after?

// check required -> optional
const isOptional = (node: Node) => node.getSymbolOrThrow().isOptional();
const incompatibleOptional = isOptional(target) && !isOptional(source);
if (incompatibleOptional) breakingReasons |= DiffReasons.RequiredToOptional;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we differentiate if it's request or response? I think if it's request, changing from required to optional would be a non-breaking.

}

// TODO: not support for overloads
function findParameterBreakingChanges(sourceMethod: Symbol, targetMethod: Symbol): DiffPair[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why I see the changelog for isUnexpected helper overloads change if it's not supported yet?

changelog.removedTypeAlias.push({ line: 'Removed Type Alias ' + name, oldName: name });
return;
}
// ignore type change, handle in a simple way for now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also seen the type alias change changelog item, I think?

@@ -1,4 +1,5 @@
export enum SDKType {
None = 'None',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a real case when we get a None type for SDK?


export function isSameSignature(left: Signature, right: Signature): boolean {
if (left.getTypeParameters().length !== right.getTypeParameters().length) return false;
if (left.getParameters().length !== right.getParameters().length) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, what is getTypeParameters and its difference between getParameters?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants