-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: port webhooks api to new switch pattern #1404
Conversation
99f5f04
to
d864ff0
Compare
fdf32ef
to
526c381
Compare
526c381
to
fccafce
Compare
679ea20
to
3465cad
Compare
); | ||
} | ||
} | ||
// async function example_API_ListWebhooks(topicClient: TopicClient, cacheName: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to comment out the example code for webhooks since the CI kept failing for it.
Will publish this sdk version and update the tests accordingly in a follow-up pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm confused as to why these would fail, the package.json in the examples dir should be pinned to a released version of the SDK. Also it's important for the sake of backward compatibility that this syntax still work, so I think we will need to get to the bottom of it before we merge this.
@@ -122,6 +122,7 @@ | |||
"version": "0.9.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this file change? just curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was experimenting with the ci build failures, I reinstalled my node modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good at a glance. just a few comments about keeping some test coverage for the old pattern, and getting to the bottom of whatever is going on with the examples
@@ -98,16 +97,17 @@ export function runWebhookTests( | |||
webhook.id.cacheName, | |||
webhook.id.webhookName | |||
); | |||
if (resp instanceof GetWebhookSecret.Success) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be good to keep at least one test around that uses the old syntax.
Description:
I had to comment out the example code for webhooks since the CI kept failing for it.
Will publish this sdk version and update the tests accordingly in a follow-up pr.
Issue
Closes https://github.com/momentohq/dev-eco-issue-tracker/issues/956