-
Notifications
You must be signed in to change notification settings - Fork 247
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
adding "nodigest" task to add non digest assets #239
base: master
Are you sure you want to change the base?
Conversation
Cool! The implementation looks good to me. Lets document it now. Could you add it to the README? |
The new task `assets:nodigest` copies compiled files removing the md5 from the file.
@@ -69,6 +82,16 @@ def define | |||
end | |||
end | |||
|
|||
desc "Compile non-digest files" | |||
task :generate_nondigest => :environment do |t, args| |
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 removed the previous files
params for the task so we can accept multiple params comma separated like:
rake 'assets:generate_nondigest[file1.js, file2.js, folder with space/app.js]'
The args.extras
will split it by ,
.
Should I go back to task :generate_nondigest, [:files] => :environment
and do the split in the task?
Will have a little more code in the task but might be a little easier to understand that the task need params. thoughts?
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 don't remember how the command looks like with the files
parameter, sorry. Could you post here?
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.
The command would be similar but AFAIK we can't pass arrays to rake
tasks, so with the files
params:
task :generate_nondigest, [:files] => :environment do |t, args|
split_files = args[:files].split ', '
...
end
And the command would be:
rake "assets:generate_nondigest['file1.js, file2.js, folder with space/app.js']"
But as you can see, we'll need to pass it as a string and split it. Code would start to get a little hairy as we'd need to make sure variations in the passed string (with different spaces counts) would still need to be correctly parsed if we decide to support folder names with spaces.
If we don't support folder with spaces, we could drop the ,
in the string and only do a files.split(' ')
:
task :generate_nondigest, [:files] => :environment do |t, args|
split_files = args[:files].split ' '
...
end
And the command would be:
rake "assets:generate_nondigest['file1.js file2.js folder_no_space/app.js']"
Whatchathink?
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 think the explicit argument would be better.
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.
done =)
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.
updated the README as well to not include ,
but only spaces in the params
Documentation added to README. |
Do we really want to design an API around passing paths to rake arguments like this? Won't you always be deploying with the same set of non-digest dependencies? Writing non-digest files to How are people going to configure separate caching semantics with these assets. They can't live forever with max-ago=10000000. |
@josh this is separate rake task to copy compiled files without the digest. Why would this interfere with manifest GC process?
Where? This is basically copying files in the
As said in the PR description, the cache expiration is different problem. This task is not the default task (but a complimentary one) and there's a brief explantation in the README telling devs that is their responsibility to deal with cache for those files. Also, that's the reason you need to explicitly pass all the files you want to get non-digest, so ppl don't overuse this. The use case for this is for JS/CSS SDKs. Files that you need to have access from outside of you application and need to have public access. See the Shopify's example. Also another fact that this is a feature that people want is the |
IMO, if you're publishing a well-known URL for a thing, you're better off serving it via the router and a controller. That way you have full control of caching policies, etc. Now, it's separately true that we don't currently make it terribly easy to have an action that's basically "render the current latest contents of asset X", so I'm not claiming this is a solved problem... but I'm personally not keen on endorsing this particular solution for the described use case. I guess I see "assets" not as "JS, CSS, etc" but as "supporting material for the application": if you have an Actually Important Thing On A URL, I'm unconvinced it's a mere "asset", regardless of what MIME type it's returning. (And yes, even if you're using the asset pipeline to assemble it.) |
You can still have control of the assets policies from the assets server I don't see great benefits from serving these assets (they are static On Monday, May 11, 2015, Matthew Draper [email protected] wrote:
|
Good point about always deploying the same assets. Maybe we should add a new config option like: |
@matthewd I think we can alo have full control of caching policies even if we skip the router. Also I think serving from the router would make harder to serve these assets from a CDN right? |
This would definitely follow existing convention better, and then this task could be tied in with the precompile process automatically without having to trigger another call. |
Problem
This gem does not generate non-digest files anymore.
A lot of apps still need the non-digest because some of their assets belongs to an SDK that they need to serve it outside of their application (e.g. like what Shopify does). For that they need to have a constant URI for their assets.
There's alternative like the non-stupid-digest-assets but I feel like this should be built-in as it's a more common pattern.
Solution
Just copy existing files without the digest part of the name.
Limitation
As we are not recompiling the file, any external file reference (image references) will still contain digested references. This is not a problem as the use case for this feature is to use the asset file as part of a SDK to be served to other developers, so references to digested files are OK.
But, but... what about cache expiration?
That's a different problem for the developer to solve per asset file. The idea behind this is to enable the developer to generate a few non-digested files, not to serve the entire application assets.
As this not rake task is not run by default during
assets:precompile
the cache issue is another problem. This is an explicit action for a common and punctual problem.cc @rafaelfranca @arthurnn for discussion
Related: #49