-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[RFC] Move Paperclip into an extension #2324
Comments
Full disclosure I directed and helped @swcraig so I'm implicitly biased but also 👍. We often don't use paperclip in all of stores, and where we do use it we (maybe) probably shouldn't. Pulling it out into an extension and preparing for the future or alternative image implementations is a 👍 for me, as long as you can drop in the gem seamlessly. |
I'd be happy as long as we can ship an image provider to new users relatively seamlessly. If we can get this so we just change our README to recommend
as the starting Gemfile and provide the same experience we have now, that would be 👍 |
We also need to consider how to cleanly extract paperclip from It's probably worth asking in slack to see how widely this is used. |
I think general consensus is that we should move off of Paperclip. There is ongoing work to add ActiveStorage support in #2974. |
Since the earliest releases of the project, image uploading and display was handled by
has_attached_file
and Paperclip.Paperclip was the first and most widely adopted file upload gem in Rails, and has done an excellent job in its 10 year history - both on its own and for the Spree and Solidus projects.
However, as web apps have evolved so have our file/image management needs and options. Not all stores use Paperclip (or
Spree::Image
) to store or manage its image assets. Many stores hack in alternative options like imgix, Contentful, Alchemy, or a host of others to handle image management and display.As far as Thoughtbot is concerned, Paperclip is in a semi-official state of non-maintenance.
Tying Solidus to a single file upload library, and one that is very likely to be unmaintained in the near future, does not seem to be an ideal situation going forward. There are alternatives to Paperclip, including ActiveStorage in Rails 5.2, and we should allow as much room as possible for people to use any of these libraries in Solidus. None of these libraries have identical feature sets, and forcing Solidus users to use any single one of these is restrictive. Pulling Paperclip out of Solidus into an extension makes room for custom implementations, and also gets rid of the ImageMagick dependency for installs.
I believe the core of Solidus needs to start extracting the Paperclip functionality into a gem, and provide a better interface for alternative image implementations.
There isn’t an easy or clear path to extract Paperclip with minimal disruption to existing stores, but I believe the path laid down here is the best starting point. It follows the pattern Clarke attempted in #625, but in a much lighterweight and gem-dependent manner.
If we can adopt the idea of a “gallery” on
Spree::Variant
andSpree::Product
, we can rely on that in core and push implementation of image handling to a suite of extensions (providing solidus_paperclip to start, and by default). Starting off this way will also allow us to move forward for now without changing any of the views of Solidus - they mostly act like they do when a variant/product doesn't have images.If we were to accept the idea of a "gallery", this proposed PR would remove all Paperclip specific code from Solidus. It is still very much a work in progress.
I'm happy to have a discussion about this path here, or feel free to ping me (@swcraig) on the Solidus slack
The text was updated successfully, but these errors were encountered: