Fix: onChange in v2 #612
Fix: onChange in v2 #612
Conversation
packages/ssm/README.md
Outdated
| @@ -57,7 +57,7 @@ npm install --save @middy/ssm | |||
| - `cacheExpiry` (number) (default `-1`): How long fetch data responses should be cached for. `-1`: cache forever, `0`: never cache, `n`: cache for n ms. | |||
| - `setToEnv` (boolean) (default `false`): Store role tokens to `process.env`. **Storing secrets in `process.env` is considered security bad practice** | |||
| - `setToContext` (boolean) (default `false`): Store role tokes to `handler.context`. | |||
| - `onChange` (function) (optional): Calls function when role tokens change after being initially set. | |||
| - `onChange` (function(data)) (optional): Calls function when role tokens change after being initially set. | |||
There was a problem hiding this comment.
role token doesn't make sense in this context
There was a problem hiding this comment.
Good catch, I also need to make another tweak to ensure it doesn't call on init.
|
@lmammino @theburningmonk I currently have it implemented in this PR to pass in the data to onChange, but it feels like a hack for the use case in #611. This is a better solution: middy(originalHandler)
.use(ssm({
fetchData:{
key:'path'
},
setToContext: true
}))
.before((handler) => {
process.env.defaults = handler.context.key.defaults
})For this specific use case I don't see a need for
Are there other known use cases that should be considered? Maybe we don't need an onChange hook right now? |
|
What about removing |
|
I agree, I think we can remove it entirely. This pattern is kinda already documented in the best practice section on the main readme. I'll update to remove the onChange, add an extra example to ssm, but leave in the hook to detect if it was pulled from cache for those building custom middleware that need to chain. |
Fixes #611