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 #6655

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

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

Specifically, during assets precompile, 'javascript_include_tag' et al are 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.

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
Member

steveklabnik commented Jun 7, 2012

This still isn't targeted at the 3.2-stable branch. :/

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.

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 changed index.* filenames into directory names inside each_logical_path, which is the inverse of the documented behavior, doesn't make any sense, as you point out, and prevents any existing assets named index.* from being precompiled,

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

Also yes I feebed up the pull request again; I plead newbishness wrt large multi-branch projects and will cheerily submit it again, this time targeted to the right right place, if that's the indicated fix.

Owner

rafaelfranca commented Jun 7, 2012

Closing this PR since it is not targeting a valid branch. Please reopen it with target to 3-2-stable.

I'd love to see this fixed. All my index.js & index.css stopped precompiling when I pulled in what I thought was just a security upgrade. I can confirm this is a bug on 3.1.5 & 3.1.6 as well as 3.2.6.

What branch should this target if it's a bug in 3.1, 3.2, and master?

Owner

rafaelfranca commented Aug 20, 2012

It should target master, so we can backport to 3-2-stable. 3-1-stable is only receiving security fixes.

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