{"action":"created","issue":{"url":"https://api.github.com/repos/camertron/generated-assets/issues/2","repository_url":"https://api.github.com/repos/camertron/generated-assets","labels_url":"https://api.github.com/repos/camertron/generated-assets/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/camertron/generated-assets/issues/2/comments","events_url":"https://api.github.com/repos/camertron/generated-assets/issues/2/events","html_url":"https://github.com/camertron/generated-assets/pull/2","id":174598662,"number":2,"title":"New logic for assets_dir filename","user":{"login":"alexey-dc","id":1108203,"avatar_url":"https://avatars.githubusercontent.com/u/1108203?v=3","gravatar_id":"","url":"https://api.github.com/users/alexey-dc","html_url":"https://github.com/alexey-dc","followers_url":"https://api.github.com/users/alexey-dc/followers","following_url":"https://api.github.com/users/alexey-dc/following{/other_user}","gists_url":"https://api.github.com/users/alexey-dc/gists{/gist_id}","starred_url":"https://api.github.com/users/alexey-dc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alexey-dc/subscriptions","organizations_url":"https://api.github.com/users/alexey-dc/orgs","repos_url":"https://api.github.com/users/alexey-dc/repos","events_url":"https://api.github.com/users/alexey-dc/events{/privacy}","received_events_url":"https://api.github.com/users/alexey-dc/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-09-01T18:54:19Z","updated_at":"2016-09-01T22:41:28Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/camertron/generated-assets/pulls/2","html_url":"https://github.com/camertron/generated-assets/pull/2","diff_url":"https://github.com/camertron/generated-assets/pull/2.diff","patch_url":"https://github.com/camertron/generated-assets/pull/2.patch"},"body":"Change it to be dependent on i18n assets instead of being randomly generated.\r\n\r\nThis helps address a caching bug that forces most assets to be rebuilt upon launching rails.\r\n\r\nThe generated_assets directory is added to app.config.assets.paths.\r\nIn sprockets, it defines the list of dependencies for \"environment-paths\" -\r\nThat means that any asset that has \"environment-paths\" as a dependency depends on the assets_dir directory.\r\nMost rails assets end up depending on \"environment-paths\".\r\nIf one of the directories in that dependency list changes, the asset has to be rebuilt.\r\nA random directory name forces asset rebuilding on each rails launch, and a static path name doesn't detect i18n assets getting changed.\r\nTo address both use cases, generate the directory name based on the mtimes of the internationalization .yml files."},"comment":{"url":"https://api.github.com/repos/camertron/generated-assets/issues/comments/244234527","html_url":"https://github.com/camertron/generated-assets/pull/2#issuecomment-244234527","issue_url":"https://api.github.com/repos/camertron/generated-assets/issues/2","id":244234527,"user":{"login":"camertron","id":575280,"avatar_url":"https://avatars.githubusercontent.com/u/575280?v=3","gravatar_id":"","url":"https://api.github.com/users/camertron","html_url":"https://github.com/camertron","followers_url":"https://api.github.com/users/camertron/followers","following_url":"https://api.github.com/users/camertron/following{/other_user}","gists_url":"https://api.github.com/users/camertron/gists{/gist_id}","starred_url":"https://api.github.com/users/camertron/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/camertron/subscriptions","organizations_url":"https://api.github.com/users/camertron/orgs","repos_url":"https://api.github.com/users/camertron/repos","events_url":"https://api.github.com/users/camertron/events{/privacy}","received_events_url":"https://api.github.com/users/camertron/received_events","type":"User","site_admin":false},"created_at":"2016-09-01T22:41:28Z","updated_at":"2016-09-01T22:41:28Z","body":"Hey @alexey-dc, thanks for the pull request. I have a couple of reservations:\r\n\r\nFirst, I don't think generated-assets is the place to make this change. Generated-assets is a general-purpose dynamic asset generation system that has nothing to do with translations. It so happens that it's often used to dynamically generate localized assets (via i18n-js-assets), but not always. For that reason, please open a pull request against i18n-js-assets and close this one.\r\n\r\nSecond, I think some additional changes will be necessary to both generated-assets as well as i18n-js-assets to realize the behavior you'd like to see. At the moment, generated-assets handles the automatic generation of the asset path. This isn't ideal since the path should be based on the mtimes (or contents) of your translation files, which generated-assets knows nothing about. I propose that we remove the logic that generates random paths and instead put that burden on the i18n-js-asset gem. Generated-assets should still place assets in a temporary directory (maybe something like /tmp/generated-assets/) but beyond that the path should be determined by i18n-js-assets.\r\n\r\nThird, instead of looking for all .yml files in config/locales, consider looping over the files listed in the i18n load path. Doing so will ensure you account for translation files from gems, engines, and other non-standard places. Furthermore, Ruby's built-in digest classes can be incrementally fed text via the `<<` operator. I'd also like to advocate for using the file's actual contents instead of the mtime, since the contents are a better predictor of actual change (I think mtime behaves differently depending on your operating system). Maybe something like this:\r\n\r\n```ruby\r\ndef asset_dir\r\n  digest = Digest::SHA256.new\r\n  I18n.load_path.each { |file| digest << File.read(file) }\r\n  File.join(Dir.tmpdir, digest.hexdigest)\r\nend\r\n```\r\n\r\nI thought reading in all the i18n files might be kind of expensive, but when I tried it on our sizable rails app with ~400 files it took 0.2 seconds. That's pretty good in my opinion.\r\n\r\nWhat do you think?"}}
{"actor":{"display_login":"camertron"}}