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

Fingerprinting images #1523

Closed
wants to merge 10 commits into
base: development
from

Conversation

Projects
None yet
3 participants
@paganotoni
Copy link
Member

paganotoni commented Jan 5, 2019

This PR adds fingerprinted images to our Webpack configuration.

In order to make it backwards compatible it will copy into public both, fingerprinted and plain name files, with this, existing applications can start transitioning to fingerprinted images and we could later on deprecate these (v0.15.x).

Looking forward for your comments on this long awaited feature, thanks for the patience!

@paganotoni paganotoni requested a review from gobuffalo/core-managers Jan 5, 2019

paganotoni added some commits Jan 5, 2019

@stanislas-m stanislas-m changed the title Figerprinting images Fingerprinting images Jan 7, 2019

paganotoni and others added some commits Jan 7, 2019

@markbates

This comment has been minimized.

Copy link
Member

markbates commented Jan 12, 2019

@paganotoni @stanislas-m what's the status of this? I think I remember some discussion in slack about it, but don't know how it was resolved.

@stanislas-m

This comment has been minimized.

Copy link
Member

stanislas-m commented Jan 13, 2019

Just checked, and it works fine.

@markbates
Copy link
Member

markbates left a comment

I tried it and got two of every image in my public folder: logo.svg and logo.Xxx.svg

That doesn’t seem right to me. Is that expected? If so, that’s strange behavior we’ll get a lot of issues about. If not, then we need to fix it.

Either way, I don’t feel comfortable merging it until it doesn’t generate duplicates.

@paganotoni

This comment has been minimized.

Copy link
Member

paganotoni commented Jan 14, 2019

@markbates duplicate images are there on purpose. This is as way to continue support to 0.13 and prior versions. Another use case for this is supporting needs like serving the image inside an email that's not rendered by buffalo mailers (there is no imgTag helper at the controller/lib level).

@markbates

This comment has been minimized.

Copy link
Member

markbates commented Jan 14, 2019

@paganotoni

This comment has been minimized.

Copy link
Member

paganotoni commented Jan 14, 2019

I don't feel the fundamentals of this one will change (p.e fingerprinting images is a win). IMO we need to get back to the drawing board to think in:

  • how are we going to transition existing users to fingerprinting ? (when they run buffalo fix)
  • to me this feature undercovers the need for a helper that we can call from actions/libraries to get the path for any fingerprinted asset.

Overall i think we need to define those 2 before merging this PR, i'm closing it until we define it.

@paganotoni paganotoni closed this Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment