-
Notifications
You must be signed in to change notification settings - Fork 12
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 cache-busting path for webshims #22
Conversation
Hmm... looking at #20 I see that there's a simpler method for getting files from a director with |
…ting with Sprockets.
@@ -37,10 +37,12 @@ Since webshims does not support fingerprinting, this will result in 404s (missin | |||
(Note that this should be run directly, not in a dom-ready block.) | |||
|
|||
```javascript | |||
$.webshims.setOptions('basePath', '/assets/webshims/shims/') | |||
$.webshims.setOptions('basePath', '/webshims/shims/1.15.6/') |
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.
Looking at this change.. I don't think it's a good idea to hardcode the webshims version into the application. That's pretty much what we are trying to avoid in all this.
Maybe we should use the webshims_path helper here as an example.
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 agree, which is why I created the webshims_path
helper. However, in our case (and this seemed likely for most implementations), this line is in a JS file that's loaded and compressed with the rest of the JS into application.js
and therefore doesn't run through ERB (or HAML or whatever).
The problem I've found, running this branch, is that the webshims are no longer offloaded to nginx and so they use the rails server which they really shouldn't need to.
I think I like your approach of just writing webshims to public and letting the assets manager (nginx, CDN, etc.) take care of it from there.
I meant to comment on this when I committed 618069c. I was trying to address the caching issue jeremy brought up, but I didn't want to run all the asset requests through Rack. (Maybe this is what Jeremy means when he says "your approach"?) Putting the assets in /public seems to be the right approach. My fix has the same issue of needing to specify the version number, but as you'll see in the Readme, i suggest adding an .erb extension to dynamically include the correct webshims version ID. Closing this pull request for now. |
David – yes, I meant your solution at 618069c. :) |
Change the path for Webshims from
/assets/webshims/shims/
to/assets/webshims/shims/1.15.6
so that when Webshims is updated, cached versions of the combos files will not be used by the browser causing incompatibilities. See #21.There are no tests in this code because you didn't have any tests in the original code so I wasn't sure what test harness you prefer. I'm happy to add the scaffolded rails app and use rspec to cover this.
Also, the Rails 4 changes are entirely speculative as I'm not using this in a Rails 4 environment to test.
I'd appreciate any feedback on this and suggestions for improvement.