Default config.assets.digests to true in development #15155

Merged
merged 1 commit into from May 19, 2014

Conversation

Projects
None yet
3 participants
@dskang
Contributor

dskang commented May 17, 2014

With rails/sprockets-rails#149, assets will only be served if the request has a digest if config.assets.digest = true and config.assets.raise_runtime_errors = true.

This PR sets config.assets.digest to true by default and will help users avoid hardcoding paths that work in development but break in production.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 17, 2014

Member

Cool! Change the Gemfile to point to sprockets-rails master and we run the test suite against it.

Member

rafaelfranca commented May 17, 2014

Cool! Change the Gemfile to point to sprockets-rails master and we run the test suite against it.

@@ -198,12 +198,9 @@ will result in your assets being included more than once.
WARNING: When using asset precompilation, you will need to ensure that your
controller assets will be precompiled when loading them on a per page basis. By
-default .coffee and .scss files will not be precompiled on their own. This will
-result in false positives during development as these files will work just fine

This comment has been minimized.

@dskang

dskang May 17, 2014

Contributor

No longer results in false positives thanks to rails/sprockets-rails#84.

@dskang

dskang May 17, 2014

Contributor

No longer results in false positives thanks to rails/sprockets-rails#84.

@dskang

This comment has been minimized.

Show comment
Hide comment
@dskang

dskang May 17, 2014

Contributor

Done!

Contributor

dskang commented May 17, 2014

Done!

@dskang

This comment has been minimized.

Show comment
Hide comment
@dskang

dskang May 17, 2014

Contributor

From a quick glance at the CI failures, it seems like there are instances in the tests that request assets without their digests and some asserts that expect the non-digested URL (e.g. expecting /src='\/\/example\.com\/assets\/rails\.png'/ when actual is "var src='//example.com/assets/rails-495b94d9cdd18b3a6eba96326cd909de.png';\n")

I'll take a look at turning off the digest flag when appropriate for some of the tests and amending the regex to include a digest for others.

Contributor

dskang commented May 17, 2014

From a quick glance at the CI failures, it seems like there are instances in the tests that request assets without their digests and some asserts that expect the non-digested URL (e.g. expecting /src='\/\/example\.com\/assets\/rails\.png'/ when actual is "var src='//example.com/assets/rails-495b94d9cdd18b3a6eba96326cd909de.png';\n")

I'll take a look at turning off the digest flag when appropriate for some of the tests and amending the regex to include a digest for others.

@@ -342,6 +335,8 @@ class User < ActiveRecord::Base; raise 'should not be reached'; end
end
RUBY
+ add_to_env_config "development", "config.assets.digest = false"

This comment has been minimized.

@rafaelfranca

rafaelfranca May 19, 2014

Member

Do we need to disable digest for this case?

@rafaelfranca

rafaelfranca May 19, 2014

Member

Do we need to disable digest for this case?

This comment has been minimized.

@dskang

dskang May 19, 2014

Contributor

Yup, we need to disable it. We make a GET to "/assets/demo.js" which doesn't work when digests are enabled since a digest isn't included in the request URL.

@dskang

dskang May 19, 2014

Contributor

Yup, we need to disable it. We make a GET to "/assets/demo.js" which doesn't work when digests are enabled since a digest isn't included in the request URL.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 19, 2014

Member

Could you add a CHANGELOG entry?

Member

rafaelfranca commented May 19, 2014

Could you add a CHANGELOG entry?

@dskang

This comment has been minimized.

Show comment
Hide comment
@dskang

dskang May 19, 2014

Contributor

Sure. Do you mean another CHANGELOG entry in addition to the one already added to railties/CHANGELOG.md?

Contributor

dskang commented May 19, 2014

Sure. Do you mean another CHANGELOG entry in addition to the one already added to railties/CHANGELOG.md?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 19, 2014

Member

Oops, it is there already. thanks.

Member

rafaelfranca commented May 19, 2014

Oops, it is there already. thanks.

rafaelfranca added a commit that referenced this pull request May 19, 2014

Merge pull request #15155 from dskang/digest
Default config.assets.digests to true in development

@rafaelfranca rafaelfranca merged commit 09cc922 into rails:master May 19, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@dskang

This comment has been minimized.

Show comment
Hide comment
@dskang

dskang May 19, 2014

Contributor

Thanks @rafaelfranca and @matthewd for your help in getting this and rails/sprockets-rails#149 done!

Contributor

dskang commented May 19, 2014

Thanks @rafaelfranca and @matthewd for your help in getting this and rails/sprockets-rails#149 done!

@dskang dskang deleted the dskang:digest branch May 19, 2014

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jun 4, 2014

Member

❤️ this so much. Thanks!

Member

schneems commented on f369bcf Jun 4, 2014

❤️ this so much. Thanks!

@mcolyer mcolyer referenced this pull request in Mange/roadie-rails Mar 1, 2015

Closed

AssetPipelineProvider fails in Rails 4.2? #36

@dnrce dnrce referenced this pull request in TracksApp/tracks Jun 10, 2016

Merged

Upgrade to Rails 4.2 #1910

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