Try to fix issue #643 "yeoman build not updating revved image references in css" #702

Merged
merged 1 commit into from Dec 3, 2012

Conversation

Projects
None yet
6 participants
Contributor

roparz commented Nov 7, 2012

src dirname can now be relative to its parent in global replace handler of usemin task

Contributor

sleeper commented Nov 7, 2012

Not sure to really understand the issue ...
Additionally it seems we're only testing agaist the parent (..) .. what about other dires up the tree ?

Could you also add dedicated tests for this ?

julrich commented Nov 7, 2012

@sleeper as I actually had this problem myself (funny to find yourself commenting the issue in question ;) ); my issue was that relative image-urls in our css were not replaced with the revved versions of the respective images.

This was due to a check here:

https://github.com/yeoman/yeoman/blob/master/cli/tasks/usemin.js#L385

which would never be true for relative file-paths for us, as in our case dirname was ../images (path.dirname(...) of the regexp match inside the css-content), whereas path.dirname(f) would be images (with f being the location grunt.file.expand found an image with the same filename as the one referenced in the css).

The PR pretty much implements the quick hack I made and posted about in #643, and you're right that this only fixes scenarios where you have a specific directory structure (css referencing files one directory up). I think it should be more flexible, to support other arbitrary configurations, especially when this will live as a separate grunt-plugin in the future.

I toyed with it a bit further, and I think I could try to make a PR tomorrow.
For relative paths in css-files it should just be checked against path.normalize([cssFileDir, dirname].join('/')), I think. With cssFileDir being the directory the css-file containing the relative url resides in.

Could just pass it down from the main registerMultiTask here:
https://github.com/yeoman/yeoman/blob/master/cli/tasks/usemin.js#L158

(rather odd btw. that usemin:post:css is registered as a helper twice, only the second is actually called).

Contributor

sleeper commented Nov 8, 2012

Yeah ... I was more thinking this way (i.e. the path.normalize stuff)... This makes more sense too me ;)
I'm not really happy with the current implementation of usemin, but I know there's currently an effort to rewrite it properly...

julrich commented Nov 8, 2012

Right. I guess that's me that's making an effort to rewrite it.

Got it more or less working with Grunt 0.4.0 now, next step is getting all the tests to run again, and then possibly refactoring.

Contributor

roparz commented Nov 8, 2012

@julrich @sleeper Thanks for taking time to answer me.
Using path.normalize look much better, but I didn't understand where you get the cssFileDir.
I can try to make a test in usemin-test.js to point the problem.

julrich commented Nov 8, 2012

The quick local hack I attempted yesterday involved just passing down this p:

https://github.com/yeoman/yeoman/blob/master/cli/tasks/usemin.js#L158

like this:
content = grunt.helper('usemin:post:' + name, content, p);
in https://github.com/yeoman/yeoman/blob/master/cli/tasks/usemin.js#L174

and likewise like this:
grunt.registerHelper('usemin:post:css', function(content, srcPath) {
in https://github.com/yeoman/yeoman/blob/master/cli/tasks/usemin.js#L353

and:
content = grunt.helper('replace', content, /url\(\s*['"]?([^'"\)]+)['"]?\s*\)/gm, srcPath);
in https://github.com/yeoman/yeoman/blob/master/cli/tasks/usemin.js#L356

and then just work with that in grunt.registerHelper('replace', function (content, regexp, srcPath) { ... }

E.g.:

var normalizedDirname = path.normalize([path.dirname(srcPath), dirname].join('/')); 
[...]
var filepath = filepaths.filter(function(f) { return normalizedDirname === path.dirname(f); })[0];

If that makes any sense. Could whip up a PR later today for that, I think. But a test would be nice either way.

Contributor

roparz commented Nov 8, 2012

I did it, and it works great.
Thanks.

julrich commented Nov 8, 2012

Looks good to me. What do you think @sleeper ?

Contributor

sleeper commented Nov 8, 2012

Seems good to me also: just one comment on the added test. When it is fixed, I guess we can merge.

julrich commented Nov 8, 2012

You're right about the test, should've looked closer at that.

I think we should add a css-file to the fixtures to test this, analogous to usemin.html:
https://github.com/yeoman/yeoman/blob/master/cli/test/fixtures/usemin.html

E.g. test/fixtures/css/usemin.css, which includes both a rule with url(../images/image.png) and one like this url(images/image.png), and then write the test-image files to the respective directories to test this.

Contributor

sleeper commented Nov 8, 2012

Yep, but I would say creating the css file and associated images, should come in addition .. The current test (with the suggested modifications) should stay as it is currently a unit test of the usemin:post:css. The additional test you proposed is more an "integration" test (i.e. make sure the tasks blend well together)

Owner

sindresorhus commented Dec 3, 2012

@sleeper What's left for this to land?

Contributor

sleeper commented Dec 3, 2012

I guess that apart from the fact the commits are not squashed it's ok.
@roparz Could you please squash the commits ?

Contributor

roparz commented Dec 3, 2012

Commits squashed.

Contributor

sleeper commented Dec 3, 2012

Thanks a lot !!!!

sleeper added a commit that referenced this pull request Dec 3, 2012

Merge pull request #702 from Roparz/master
Try to fix issue #643 "yeoman build not updating revved image references in css"

@sleeper sleeper merged commit 50dd2ba into yeoman:master Dec 3, 2012

1 check passed

default The Travis build passed
Details

Thank you so much! I just ran into this problem yesterday and BOOM there's a new update to the repo fixing the issue. You guys are great. Thanks again.

Also for anyone like me who might need to know, to use the very latest version of yeoman from the repo you can simply run: npm update yeoman -g (because it is updating based on the npm registry and not the git repo directly). Here is the FAQ explaining how to use the latest commit from master instead: https://github.com/yeoman/yeoman/wiki/Additional-FAQ

Issue Description:
I installed Yeoman using the latest commit to resolve this issue. I ran 'yeoman build' successfully a few times and experienced no broken image references. Once I merged my designer's changes into the project, I experienced another revved image error. My investigation led to a relative image reference in SCSS, notably two directories up (e.g. 'url(../../images/').

Trying to fix it myself, I uninstalled/reinstalled Yeoman again, followed the instructions in Issue #796, and ran 'git reset --hard SHA: b38b905'. The errors persist.

Error info and my system details are below.

The revved image:
images/square_bg.png >> 136830f8.square_bg.png

'yeoman build' error:
Error: /images/square_bg.png Not Found
at Object.exports.error (/Users/justinwebb/Dev/apps/yeoman/cli/node_modules/connect/lib/utils.js:43:13)
at Object.errorHandler as handle
at next (/Users/justinwebb/Dev/apps/yeoman/cli/node_modules/connect/lib/proto.js:190:15)
at exports.send (/Users/justinwebb/Dev/apps/yeoman/cli/node_modules/connect/lib/middleware/static.js:157:11)
at Object.oncomplete (fs.js:297:15)
SyntaxError: Parse error

'ack square_bg.png' result:
styles/compass_twitter_bootstrap/_mixins.scss
479: background: url(../../images/square_bg.png) repeat center center;

System Details:
Mac OS X 10.8.2

'echo $PATH $NODE_PATH' result:
/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin:/usr/local/git/bin

'brew doctor' result:
Your system is raring to brew.

'node version' result:
{ http_parser: '1.0',
node: '0.8.9',
v8: '3.11.10.22',
ares: '1.7.5-DEV',
uv: '0.8',
zlib: '1.2.3',
openssl: '1.0.0f',
npm: '1.1.61' }

Dear JustinWebb,

As I see it these are the commands you need to run, in this order:

1.) Completely uninstall yeoman:
a.) sudo rm -rf ~/.yeoman
b.) sudo rm -rf ~/.npm/yeoman*
c.) sudo rm -rf /usr/local/lib/node_modules/yeoman
d.) sudo rm -rf ~/yeoman/ <-- this was the directory containing the clone of the master branch
e.) npm cache clean yeoman
f.) sudo npm uninstall yeoman -g
2.) Reinstall by cloning the master branch of Yeoman into ~/yeoman/:
a.) cd ~
b.) git clone https://github.com/yeoman/yeoman.git
3.) Reset the master branch back to the commit we need (b38b905):
a.) cd ~/yeoman
b.) git reset --hard b38b905
4.) Finish installing and linking yeoman:
a.) cd ~/yeoman/cli
b.) sudo npm install -g
c.) sudo npm link
5.) Open the package.json file in a text editor and change the yeoman version number to '0.9.7' to stop auto update:
a.) cd ~/yeoman/cli
b.) open package.json
c.) modify version number to read '0.9.7'
d.) save and overwrite the file
6.) Now go back to your project directory and try running yeoman build again to see if it's working.

Did you do everything in here? If not, or if you're not sure, try it again. If it still doesn't work, please report where you're having trouble and I'd be happy to try and help.

Good luck,
Bests,
Thomas

Yes, this is the process I followed before posting. Just to make sure, I did it again, following this script exactly. I experienced the same error, though from a different location:

Error: /images/square_bg.png Not Found
at Object.exports.error (/Users/justinwebb/yeoman/cli/node_modules/connect/lib/utils.js:43:13)
at Object.errorHandler as handle
at next (/Users/justinwebb/yeoman/cli/node_modules/connect/lib/proto.js:190:15)
at exports.send (/Users/justinwebb/yeoman/cli/node_modules/connect/lib/middleware/static.js:157:11)
at Object.oncomplete (fs.js:297:15)
SyntaxError: Parse error

Let me know else I can help.

On Dec 18, 2012, at 5:56 PM, sample-this notifications@github.com wrote:

Dear JustinWebb,

As I see it these are the commands you need to run, in this order:

1.) Completely uninstall yeoman:
a.) sudo rm -rf ~/.yeoman
b.) sudo rm -rf ~/.npm/yeoman*
c.) sudo rm -rf /usr/local/lib/node_modules/yeoman
d.) sudo rm -rf ~/yeoman/ <-- this was the directory containing the clone of the master branch
e.) npm cache clean yeoman
f.) sudo npm uninstall yeoman -g
2.) Reinstall by cloning the master branch of Yeoman into ~/yeoman/:
a.) cd ~
b.) git clone https://github.com/yeoman/yeoman.git
3.) Reset the master branch back to the commit we need (b38b905):
a.) cd ~/yeoman
b.) git reset --hard b38b905
4.) Finish installing and linking yeoman:
a.) cd ~/yeoman/cli
b.) sudo npm install -g
c.) sudo npm link
5.) Open the package.json file in a text editor and change the yeoman version number to '0.9.7' to stop auto update:
a.) cd ~/yeoman/cli
b.) open package.json
c.) modify version number to read '0.9.7'
d.) save and overwrite the file
6.) Now go back to your project directory and try running yeoman build again to see if it's working.

Did you do everything in here? If not, or if you're not sure, try it again. If it still doesn't work, please report where you're having trouble and I'd be happy to try and help.

Good luck,
Bests,
Thomas


Reply to this email directly or view it on GitHub.

Contributor

sleeper commented Dec 19, 2012

Justin,

Your issue is coming from the way the usemin task in head is working with retive path reference.

Actually, we extracted it our from the yeoman base in it's own grunt contrib, and I hope to have solved these kind of issues (and [tested them[(https://github.com/yeoman/grunt-usemin/blob/master/test/test-usemin.js#L103)).

So I would say that unfortunately you have to wait for the inclusion of this contrib to the master version of yeoman (we're currently in the process of hooking up all the different contribs).

sleeper added a commit that referenced this pull request Apr 24, 2015

Merge pull request #702 from Roparz/master
Try to fix issue #643 "yeoman build not updating revved image references in css"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment