Looking for feedback on my new cloud storage plugin before I tidy up and publish! 🎉

[richardvanbergen]

Hello all,

I've made a little plugin, currently it only supports S3 compatible APIs (like digital ocean) but I thought it'd be nice to support some other cloud service providers so I found a way of making it more generic.

There are a lot of improvements I'd like to make. For one I've only tested it in it's original S3-only form but I'd like to get a couple of comments before I go adding tests and real documentation:

1. Do you think this would work better as a more focused S3 plugin?
2. Any way I can improve the API? In particular I don't like passing in the getter function to the plugin init but I can't think of a better way because depending on your region and the service base URL, the media endpoint can be very different so we need the user to calculate it.

I also need to deal with thumbnail images. Currently it only fetches the full size image which is a bit clumsy.

https://github.com/richardvanbergen/payload-plugin-cloud-storage

[Ontopic]

Just had time for a quick glance, but looks at least like a very clean example for the (first?) community plugin, don't know much about the S3 layer. On my watch list to study better later.

[richardvanbergen]

Thanks!

I'll start adding some tests and documentation to it today. I'm starting to think that maybe I should get rid of the cdn getter and just leave that up to the user to add to their own hooks as they might not even need it depending on the provider.

Sorry, just thinking out loud 😊

[richardvanbergen]

Update: found a better solution I think. I'm allowing the user more freedom in how they create virtual fields and specify the admin thumbnail

https://github.com/richardvanbergen/payload-plugin-cloud-storage/tree/feat/append-arbitary-fields

[richardvanbergen]

Yes I went that direction, it's now merged in

[jmikrut]

Hey @richardvanbergen this is amazing!

I will be doing my best to review today. Working on our own Form Builder plugin at the moment but this is super exciting and we will be using this in our own projects as well!

One note, I just deployed a new patch that removes the requirement that plugins should return a fully sanitized config. This is handy mainly in cases where you want to inject new collections without having to go through and sanitize them fully yourself. Now your plugins can write configs just as the end user can, and the whole plugin-enabled config will get re-sanitized after plugins are executed.

I don't think this should interrupt your typings, but I'm not sure. You may need to convert your plugin's types to Config instead of SanitizedConfig.

I wrote up some notes in the changelog that reflect this as well.

[richardvanbergen]

One note, I just deployed a new patch that removes the requirement that plugins should return a fully sanitized config

That would be really nice actually. I've been doing some jest testing and making every property on SanitizedConfig required results in a lot of @ts-ignore. It's also really hard to properly mock PayloadRequest.

https://github.com/richardvanbergen/payload-plugin-cloud-storage/blob/main/src/plugin.test.ts#L109

Please let me know if you have any comments on the planned API in the meantime I'm going to setup the publish workflow and work in a way to deal with sizes.

[richardvanbergen]

Got the npm publish working, now testing in my project to make sure it actually works.

[jmikrut]

Hey @richardvanbergen — just had a chance to look through your code in-depth. It looks great! It's super inspiring to see you digging in to this plugin infrastructure so quickly and apparently effortlessly.

I really like the adapter pattern, specifically. The hooks are also nice and clean.

Here are a few of my other thoughts:

uploadCollectionModifiers

How come this is necessary? As the user is free to write their own config in advance of installing this plugin, wouldn't they be free to just add fields / add their adminThumbnail function right into their own config(s)? I'm probably just missing something here.

cloudStorageUrl Field

If the cloudStorageUrl field was not supplied by the user, would it be up to their own frontend(s) to manually construct the cloud storage URLs? I feel like this will be so frequently necessary that it might be a better pattern to just include the "logic" of building those URLs into the adapter pattern. Or at least.... hmm. Maybe just add a plugin option for the afterRead hook to generate the cloud storage URL and then take care of adding the field itself for the user.

Just trying to simplify the amount of work that the end user needs to do to get this plugin working. This will all be doubly complicated once we support sizes, which at that point, it might be much more warranted to simplify the upfront work for the end user.

I think that's really all that jumped out at me! Again, that adapter pattern is super nice and I am really looking forward to seeing this development continue. Want our help anywhere?

[jmikrut]

Also - @richardvanbergen we just subtly modified the order that config plugins are instantiated, which has both performance improvements and opens up more possibilities with plugins.

See this breaking change for more info. I skimmed your code and at first pass you are already accessing everything safely (thanks TypeScript), so you should not need to make any changes. But if you need to make changes, will you let me know?

[richardvanbergen]

Hello @jmikrut, first of all, thanks for taking the time out your busy day to review this for me!

As the user is free to write their own config in advance of installing this plugin, wouldn't they be free to just add fields / add their adminThumbnail function right into their own config(s)?

Absolutely they can, the parameter uploadCollectionModifiers and it's subfields are completely optional. But if they're like me they might have multiple upload collections and being able to specify the virtual fields on the plugin configuration itself prevents repeated code and keeps naming consistent across collections.

It also means that if they add new upload collections get added they will inherit those configs automatically.

I feel like this will be so frequently necessary that it might be a better pattern to just include the "logic" of building those URLs into the adapter pattern.

I think that there is enough variation in how the URLs could be structured that it would be unwise to assume a given URL structure.

For instance:

• The DO Spaces API is an S3 compatible API but the endpoints for your files obviously follow a different pattern to actual S3.
• What if the user wants to do something like use cloudflare or cloudfront as their CDN?

So for me to take this approach I would have to create separate adapter implementations for both DO and AWS and allow for an override in case it needs to be overridden.

Or at least.... hmm. Maybe just add a plugin option for the afterRead hook to generate the cloud storage URL and then take care of adding the field itself for the user.

This is the approach that I originally had but in my case I need to be able to change the field name to maintain backwards compatibility and rather than adding a dozen little customisation options for field name, label, access, options I figured I would just use what payload already provides.

So taking all this feedback in account, what I could maybe do is:

1. Add a default configuration, which requires a getEndpoint function to be passed in.
2. Allow for the user to override the field configuration entirely which will remove the requirement for getEndpoint.
3. Allow the user to pass in uploadCollectionModifiers.fields: false which will disable this behaviour.
4. Use the endpoint function to determine the uploadCollectionModifiers.adminThumnail by default.

I could also go one step further and separate the adapters for S3 and DO from each other, which would make getEndpoint optional. I'm not so convinced about that though.

See this breaking change for more info. I skimmed your code and at first pass you are already accessing everything safely (thanks TypeScript), so you should not need to make any changes. But if you need to make changes, will you let me know?

Of course, I will be happy to bug you if you broke my plugin! 😁

Once again thank you very much for the feedback.

[richardvanbergen]

OK I've started to implement some suggestions, The branch is currently broken but I've made all the progress I can for today. The documentation at least now describes what I'm going for.

https://github.com/richardvanbergen/payload-plugin-cloud-storage/tree/feat/api-feedback-refactor

[Ontopic]

Really swamped, but when I got time for My First Payload-plugin™ I'm definitely gonna use yours as a boilerplate / starter. Maybe the boys from TRBL (easiest name to remember for me ever 👍 ) can have a look at the structure and release a plugin-template?

[richardvanbergen]

I was actually thinking about doing that myself too. I thought it'd be fun to look into yarn create payload-plugin.

And thanks for the kind words! I have to have the first version finished this week.