feat: allow buildPlugin to accept array of secret names #138
Conversation
There was a problem hiding this comment.
Good job so far!
I would definitely go with pMap and introduce a new dep. the implementation would be extremely easy (conditionally call either pProps or pMap based on whether opts.secrets is an array or not) and it would avoid the index properties which a little ugly.
I tried with that and it gives the same behaviour as |
Ok I wrote pMap just as a reference to what you mentioned earlier, but maybe pMap is not the right one. Here we're getting an array as input and we want to turn it into an object, right? Hence, the operation is a reduce operation, meaning, p-reduce https://github.com/sindresorhus/p-reduce |
test/build-plugin.test.js
Outdated
| t.ok(decorate.called, 'decorates fastify') | ||
| t.ok( | ||
| decorate.calledWith('secrets', { | ||
| 0: 'content for secret1-name', |
There was a problem hiding this comment.
I would not expect these numbers properties, don't you think they are a surplus?
lib/build-plugin.js
Outdated
| @@ -33,6 +33,12 @@ function buildPlugin(Client, pluginOpts) { | |||
|
|
|||
| const secrets = await pProps(opts.secrets, (value) => client.get(value), { concurrency }) | |||
|
|
|||
| if (Array.isArray(opts.secrets)) { | |||
| for (let i = 0; i < opts.secrets.length; i++) { | |||
| secrets[opts.secrets[i]] = secrets[i] | |||
There was a problem hiding this comment.
Moving this logic before the pProps call to compose an input parameter, would remove the index properties from fastify.secrets.
I think it would be more compliant to the principle of least astonishment
There was a problem hiding this comment.
I have these index properties here for backwards compatibility. Do we not want to maintain backwards compatibility? This way, code like fastify.secrets[0] would still work
|
Before releasing this change we should consider implementing sindresorhus/p-reduce#4 and using concurrency in the p-reduce usage as well |
closes #136
I took this approach so as to make this change backwards compatible:
fastify.secrets, when we pass an array of secret names tobuildPlugin, will now have keys that are integers (previous behaviour), as well as new keys which are the actual names of the secrets.pMapmight have been useful instead ofpProps, but because of wanting to maintain backwards compatibility and not wanting to add a dependency, I went with this approach.Please let me know if you feel it is necessary to add tests for other permutations and combinations: I just added a test for the case where there is no namespace.