jonkemp / gulp-inline-css

Inline linked css in an html file. Useful for emails.
MIT License
272 stars 29 forks source link

Fix for issue #13: Stylesheet paths are not always relative #14

Closed zachrose closed 10 years ago

jonkemp commented 10 years ago

I'm not clear on what the problem and what the fix does.

Also, is there anyway to add tests for this?

zachrose commented 10 years ago

Sure thing. I've just added a test that hopefully explains the issue. (Take out JSON.parse/JSON.stringify in index.js to see the test fail.)

I'm personally not sure how on how to add a test that in effect says, "also don't do this other thing that was never expected in the first place," but for now I've just added it as a second test case with its own fixtures. I welcome any suggestions on how to do this better.

jonkemp commented 10 years ago

Ah, I see what the issue is. Thanks for pointing that out.

I don't see the need to immediately stringify and then parse the args. Why did you add that?

zachrose commented 10 years ago

JSON.parse(JSON.stringify(object)) is just the 5-cent way to clone an object in JavaScript. Objects are passed by reference, so if I do:

foo = {a: 2};
bar = foo;
bar.a = 1;
assert(foo.a == 2);

...you will see that foo.a is not 2, because bar is just foo by another name. Cloning is the only way to get a copy of foo that doesn't mess with bar. There are many suggested ways to clone an object in JavaScript. My usual way to do it is to require Underscore and do this:

var _ = require('underscore')
var original = {a: 1};
var clone = _.extend({}, original);

...and I would be happy to do that here instead if you prefer.

jonkemp commented 10 years ago

Ok. Please squash the commits. Then I'll merge it.

jonkemp commented 10 years ago

Thanks for tracking this down and providing a fix. This issue has been resolved outside of this PR however, so this is no longer needed. Closing.

zachrose commented 10 years ago

Ah, sorry I forgot to get to it. Thanks for closing!