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.

Sign up for GitHub

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

Jump to bottom

feat: user id map in MigrationExchangeResponse as map #1821

Merged
merged 1 commit into from
Jun 17, 2024
Merged

feat: user id map in MigrationExchangeResponse as map #1821

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions packages/web-api/src/types/response/MigrationExchangeResponse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ export type MigrationExchangeResponse = WebAPICallResult & {
ok?: boolean;
provided?: string;
team_id?: string;
user_id_map?: UseridMap;
user_id_map?: { [key: string]: string };
warning?: string;
};

export interface UseridMap {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ BREAKING CHANGE

If anyone was using that interface. Though given it was not very useful and cast was needed anyway, removing usage would be enough to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

How exactly is this a breaking change? Because the values within user_id_map were typed to any before, and now would be a string?

Copy link
Member

Choose a reason for hiding this comment

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

I don’t consider this a breaking change. The auto-generated types here have been changed based on quicktype outputs over time. We've been trying to maintain backward compatibility as much as possible, but when an existing type is incomplete and/or invalid, changes like this can happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Perhaps in the future, if we start paying closer attention to the response types, and start augmenting them with manual types, we could employ a more stringent backwards compatibility policy for the response types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's technically a breaking change regarding to type definitions as:

  1. Interface UseridMap is exported. It's also included in published dist/ files. So it's part of public API surface
  2. Interface UseridMap gets removed. Which is a breaking change in case someone was importing it & using it.
  3. Type definition is narrower. Before it was very open (any non null type). Now it's an indexed type which is a stricter type. So it's also non backwards compatible.

But given the type needed a cast anyways and cast was needed it's probably not enough for a new major version bump due to this kind of small breaking change. And more if auto-generated types are following a good-enough approach.

I'm happy with whatever you decide, reasons sound fair enough to me :)

}
Loading