-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
add support for actionOptions for provider upload and delete functions #9219
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
add support for actionOptions for provider upload and delete functions #9219
Conversation
Supports options object in providerOptions which can contain any cloudinary upload/delete options
Codecov Report
@@ Coverage Diff @@
## master #9219 +/- ##
=======================================
Coverage 34.64% 34.65%
=======================================
Files 1308 1308
Lines 14431 14452 +21
Branches 1432 1438 +6
=======================================
+ Hits 5000 5008 +8
- Misses 8517 8529 +12
- Partials 914 915 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
|
||
| module.exports = { | ||
| init(config) { | ||
| cloudinary.config(config); |
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.
the config here is the providerOptions you cannot access the options if they are set outside
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 not sure I understand the requested change. This is tested working as submitted and was just the least invasive way to add the options. If desired, I could possibly split the upload and delete options out to their own key at the upload config level and pull them out separately?
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 misread the example config and thought the key was on the upload config level :)
The naming might be improved I think, options doesn't say much about where it is going to be used :)
make sure to also omit the key from the config you pass down to the cloudinary sdk too ;)
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 open to any suggestions for naming.
As for the config object, would you be open to splitting the providerOptions object into another set of keys for providerOptions.cloudinaryConfig and perhaps something like providerOptions.cloudinaryOptions for the upload/delete options? It just makes the whole object split much more explicit rather than trying to omit a key from the unit function.
Thanks again for taking a look at this PR. This feature will really help my particular use case where I want to use one cloudinary account for both staging and production environments without mixing the files together in root directory (options allows me to specify a folder per environment in this case to keep things organized and separate)
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 would avoid splitting for now as it will be a breaking change.
We could add another key next to the providerOptions that would be used to wrap the provider actions when we initialize it in the createProvider at packages/strapi-plugin-upload/config/functions/bootstrap.js
sth like:
const createProvider = ({ provider, providerOptions, actionOptions }) => {
const providerInstance = require(`strapi-provider-upload-${provider}`).init(providerOptions);
return Object.assign(Object.create(baseProvider), {
...providerInstance,
upload(file, options = actionOptions.upload || {}) {
return providerInstance.upload(file, options);
},
delete(file, options = actionOptions.delete || {}) {
return providerInstance.delete(file, options);
},
});
};with this config
upload: {
provider: '',
providerOptions: {},
actionOptions: {
upload: {},
delete: {}
}
}This would make this work for any provider that wants to implement something with it. What do you think ?
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 love it, and agree on the breaking change, I'll add that and submit shortly
used to initialize upload/delete custom options which can be passed to a provider's upload/delete function
added actionOptions description with link to available cloudinary upload/delete options
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.
Last thing required :)
Add actionsOptions default value ({}) in the plugin /config/config.json
|
Hey @bglidwell would you be willing to add this to some of the documentation on the doc repo ? @derrickmehaffy might give you good pointers on where to add it ;) |
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.
LGTM 👍
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: https://forum.strapi.io/t/strapi-cloudinary-upload-presets-help/1282/7 |
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: |
|
Is there any possibility to configure this at the model attribute level? |
Supports options object in providerOptions which can contain any cloudinary upload/delete options
Linked Issues
#5069
What does it do?
Adds support for options in cloudinary plugin config
Why is it needed?
setting cloudinary options such as a default folder per environment
Related issue(s)/PR(s)
Let us know if this is related to any issue/pull request