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

fix: add missing properties to ExportChannelStatusResponse #1138

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rchl
Copy link

@rchl rchl commented Jun 20, 2023

CLA

  • I have signed the Stream CLA (required).
  • Code changes are tested

Description of the changes, What, Why and How?

Those properties were missing in the type returned from getExportChannelStatus API.

Some references:

Unfortunately there is no documentation as to what exact values the status can have so it's just typed as string.

The error and result objects could also have better types but apart from REST documentation having some info on those, it's not 100% clear on which properties are optional.

EDIT: Added types for result also, making best effort with available information.

Changelog

  • fix: add missing properties to ExportChannelStatusResponse

@rchl
Copy link
Author

rchl commented Jun 23, 2023

Also created ExportChannelsResult type to improve slightly more.

src/types.ts Outdated
Comment on lines 394 to 395
error?: ExportChannelsResult | null;
result?: {} | null;
Copy link
Member

Choose a reason for hiding this comment

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

result should use ExportChannelsResult

and error should use ErrorResult

where ErrorResult is

type?: string;
description?: string;
stacktrace?: string;
version?: string;

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. That was embarrassing :)

Applied your suggestion with the difference that I've called the error type ExportChannelError since I thought ErrorResult would be too generic. Can change if you actually want it to be that generic.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, also I think null is unnecessary here . ExportChannelError sounds good to me 👍

Copy link
Member

Choose a reason for hiding this comment

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

and we would need to make it

 status?: string;
 task_id?: string;

Copy link
Author

@rchl rchl Nov 29, 2023

Choose a reason for hiding this comment

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

and we would need to make it

 status?: string;
 task_id?: string;

Don't fully understand. Add those to the ExportChannelError or change required to optional in ExportChannelStatusResponse?

Copy link
Author

Choose a reason for hiding this comment

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

I could see the null value when testing exporting locally.

Copy link
Author

Choose a reason for hiding this comment

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

Status {
  task_id: 'c1e57bb1-04f1-4284-af61-455c3990b631',
  status: 'waiting',
  created_at: '2023-11-30T12:42:46.266232Z',
  updated_at: '2023-11-30T12:42:46.266232Z',
  duration: '16.19ms',
  result: null
}
Status {
  task_id: 'c1e57bb1-04f1-4284-af61-455c3990b631',
  status: 'pending',
  created_at: '2023-11-30T12:42:46.266232Z',
  updated_at: '2023-11-30T12:42:46.501436Z',
  duration: '3.45ms',
  result: null
}
Status {
  task_id: 'c1e57bb1-04f1-4284-af61-455c3990b631',
  status: 'completed',
  created_at: '2023-11-30T12:42:46.266232Z',
  updated_at: '2023-11-30T12:42:46.689775Z',
  duration: '2.00ms',
  result: {
    url: 'https://...'
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

I've added duration though since it was missing.
I don't know how to get ExportChannelError but I've added there too as I'm assuming it would also be present.

Copy link
Author

Choose a reason for hiding this comment

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

BTW. I wonder what makes you think that task_id and status are optional. Did you verify internally that it can be the case that those are missing? I would think that the response would be pretty useless without those.

Copy link
Author

Choose a reason for hiding this comment

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

We're closing on a year here.
If you don't care then I can just close it.

rchl and others added 9 commits November 29, 2023 16:12
* master: (44 commits)
  chore: release v8.14.4 (GetStream#1203)
  fix: add default contentType as multipart/form-data in sendFile (GetStream#1202)
  chore: release v8.14.3 (GetStream#1200)
  fix: use postForm instead of post for sending files (GetStream#1199)
  chore: release v8.14.2 (GetStream#1197)
  fix: reload channel state if frozen flag changed (GetStream#1196)
  chore(release): v8.14.1 (GetStream#1195)
  Update user reference in channel read data (GetStream#1194)
  chore(release): v8.14.0 (GetStream#1193)
  feat: axios upgrade to v1 (GetStream#1192)
  Revert "fix: queue channel WS events until the channel is initialized" (GetStream#1189)
  chore(release): v8.13.1 (GetStream#1188)
  fix: undefined values in query params (GetStream#1187)
  chore(release): v8.13.0 (GetStream#1186)
  feat: support for SNS (GetStream#1185)
  chore: release v8.12.4 (GetStream#1184)
  fix: evaluate channel.lastRead when channel is not initialized (GetStream#1183)
  chore(release): v8.12.3 (GetStream#1182)
  fix: queue channel WS events until the channel is initialized (GetStream#1179)
  chore: release v8.12.2 (GetStream#1181)
  ...
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.

None yet

3 participants