Include version as a part of the asset digest
#478
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
|
jacobbednarz
force-pushed
the
add-version-back-to-digest
branch
from
803eb42 to
4d6ed78
Compare
jacobbednarz
force-pushed
the
add-version-back-to-digest
branch
from
4d6ed78 to
fccb05f
Compare
I don't think this was testing what it thought it was.
| @cached_version_digest.dup | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Why put this in a module? I don't like modules in general, this looks like a "bad module"
There was a problem hiding this comment.
This is actually about the third iteration of the changes in this one PR which might be partially to blame for the confusion :P
Initially, this started out as just a straight port of #404 but that broke the integrity attribute we were using for subresource integrity. I played around with a few things, rebased this branch to remove all the guff and then realised the version wasn't being incremented for all assets. I finally managed to stumble onto sellect#1 which correctly solved the integrity issue so I rebased that in but now the JS filenames are including the version so I'm back to investigating that.
For now, it's probably safe to ignore this. I might even end up closing this until I solve the underlying issue here and work out if it's in sprockets or our app.
There was a problem hiding this comment.
@jacobbednarz thanks for working on this.
It's been a while since I looked at or thought about this, but I presume I was trying to follow the code patterns from previous releases as much as possible.
Let me know if I can answer any further questions or move this along in any way.
There was a problem hiding this comment.
just noticed the issues i'm seeing with it not being included are for index files (such as index.js). @kilgore5 have you happened to have tried this in your setup?
|
I'm fine with this in general. Can you rebase and get tests working and then I can merge in? |
|
I'll admit I've been neglecting this one since we hit some stumbling blocks with the |
|
@jacobbednarz sorry that I lost track of your comment. is there anything unique about your |
Updates the file digest to take into account the configured version
which allows tasks like invalidating all assets possible again.
Backports #404 to 3.x using patch from sellect#1
cc @lime who initially made the changes for
master.