Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Index js css fix #6653

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

Issue #3993 was about Sprockets not acting as documented in production environments.

Specifically, during assets precompile, 'javascript_include_tag' is supposed to notice when the included path is a directory, and use an 'index.js' or 'index.css' file from inside that directory as a manifest:

http://guides.rubyonrails.org/asset_pipeline.html#using-index-files

#3993 was closed when a fix (SHA: df84577) was merged. The problem is that the fix is not only incorrect, it actually introduced a new bug.

The fix as merged converts paths pointing to index.* files into paths pointing at their enclosing directories. This is the opposite of the documented behavior -- if the original path is a directory, 'index' needs to be added to the path to find the file inside the directory.

That commit therefore broke asset precompile such that any existing asset files named 'index.(js|css)' are completely skipped over, without fixing the original issue. This commit fixes the new bug and makes the feature work as documented, actually resolving #3993.

Russell Pickett Revert broken upstream "fix" to sprockets; add correct fix
Rails upstream got a "fix" to an issue with Sprockets not acting as
advertised with respect to directories and automatically finding
index.* files if appropriate during assets precompile.  The "fix"
didn't fix that issue, and instead created a new one where existing
index.* files got ignored.

This commit removes the broken fix, and creates a correct one.

Rails issue:
#3993

Incorrect fix:
df84577
424da34
Member

steveklabnik commented Jun 7, 2012

This code isn't even in Rails anymore, it's in sstephenson/sprockets, so you'll have to open a pull request there.

Member

steveklabnik commented Jun 7, 2012

... unless you were trying to make this change against 3.2-stable, and not master. In which case a PR should be directed at this branch, feel free to re-open and change that if you can.

Aah yeah, that was my intent. My bad, re-submitting to the correct branch now. Thanks.

each_logical_path doesn't operate with directories, each logical path is always file.

http://guides.rubyonrails.org/asset_pipeline.html#using-index-files

The documentation at the link above says that specifying assets by directory name is supported, and that the intended behavior is that it use any index.(css|js) files from that directory as a manifest.

The previous commit changes index.* filenames into directory names inside each_logical_path, which is the inverse of the documented behavior. As you point out. it doesn't make any sense because each_logical_path doesn't work with directories. And in any case, it introduces a new bug -- it prevents any existing assets named index.* from being precompiled which is how I got here in the first place.

This commit changes directory names into index.* filenames inside each_logical_path, as the documentation describes.

Mange commented Jun 7, 2012

subscribing

Owner

jeremy commented Oct 2, 2012

Fixed @ 19987b6

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