-
Notifications
You must be signed in to change notification settings - Fork 4
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: @eventual/slack-integration #89
Conversation
@@ -1,6 +1,6 @@ | |||
import "@eventual/entry/injected"; | |||
|
|||
import { createApiHandler } from "@eventual/core"; | |||
import { ApiRequest, createApiHandler } from "@eventual/core"; |
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 think we can just standardise on using the Request
type from the fetch api?
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 think we can just standardise on using the
Request
type from the fetch api?
I was thinking that too but i don't think it's compatible with Itty?
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.
Oh that's right, the node Request type doesn't have params
credentials: new JsonSecret<SlackCredentials>( | ||
new AWSSecret({ | ||
secretId: process.env.SLACK_SECRET_ID!, | ||
cacheConfig: { | ||
enabled: false, | ||
}, | ||
}) | ||
), |
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.
nit: is the wrapper needed? AWSSecret.json<SlackCredentials>()
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.
Possible but it's also a bit shitty to make type-safe. Typing the input is easier than creating an abstract class with a asJson
method that ensures the base-secret is a Secret<string>
.
this.client = | ||
props.client ?? (_defaultClient ??= new SecretsManagerClient({})); |
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.
use the memoization system the rest of them use?
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.
What would that do for me? Isn't this simple enough?
new Date().getTime() - this.#value.refreshTime.getTime() > | ||
this.cachingConfig.ttl) |
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.
Instead of refresh time, could you store expire time?
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.
Not sure what the difference is?
Partial #92