-
-
Notifications
You must be signed in to change notification settings - Fork 77
Conversation
@UltCombo Want to test this in Win? I think it should work, but it will have Windows style paths. |
...aaaaand now it should have the correct browser-style paths on Windows too. |
@robwierzbowski No problem, I'm testing right now. The path separators are correct, but the relative paths seem to be going a couple directories above than it should. I'll prepare some test cases and read the source to see if I can figure out the issue. |
If you give me a source glob, dest, and file structure I'll test on my side. |
@robwierzbowski alright, you can clone https://github.com/UltCombo/gulp-ruby-sass-maps-test |
Ah, it's a __dirname issue. Thanks. |
Ok, @UltCombo, give that a spin. Tested on an external project, should be working well. |
I think next on the list of things to do is update our tests to better mirror a real usage situation. :P. |
@robwierzbowski looks better, but seems like there is still an extra |
Forked your project, added an index for testing: https://github.com/robwierzbowski/gulp-ruby-sass-maps-test/tree/7c419e7a8f2ace6741464f16d79d67c48cec894e |
@robwierzbowski By any chance, did you serve the repo root in the root of your web server? If so, the extra |
Picturing @robwierzbowski at the moment: No, but seriously, I can take a deeper look at this in case you're too tired/fed up with the sourcemapping stuff. |
Haha. Well there are some complications that I didn't realize. Working through them now. I'd like to get this and the other issue wrapped up and then release a new version ASAP, then just let it coast for a while. |
Oh I see, v0.6.0 looks really promising and the source maps are almost functional. Very nice job so far. |
Good work on fixing this source map issue...looking forward to it being merged |
var destFile = path.join(destDir, relativeFile); | ||
|
||
// normalize to browser style paths if we're on windows | ||
return slash(path.relative(destFile, srcFile)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using uri-path
instead of slash
in order to properly encode the relative URL paths. See these discussions: gruntjs/grunt-contrib-uglify#222 (comment) and gruntjs/grunt-contrib-uglify#175 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, thanks.
After working on this with a couple more possible setups, I think the best solution is just to tell the user to supply the relative path to their source files. Revisions, examples, and reasoning to come. |
@robwierzbowski I'm okay with that, your suggestion also solves use cases where the webserver URLs do not correspond to the file system's. The user can do |
@robwierzbowski if I understand correctly, then your current suggestion would be supplying the var cssSrc = 'scss',
cssDest = 'public/css',
globbingPattern = cssSrc + '/*.scss';
var sources = glob.sync(globbingPattern).map(function(srcPath) {
return uriPath(path.relative(cssDest, srcPath));
});
gulp.src(globbingPattern)
.pipe(sass({
sourcemap: true,
sources: sources
}))
.pipe(gulp.dest(cssDest)); Though, now this seems very fragile. If the sources array is not in the correct order, the tokens will mapped to the wrong files. Maybe a callback function to treat the Though, I wonder what are complications about this relative path resolution. Would you mind if I try to help fix/workaround this issue? |
That's not really my suggestion, and I don't think it will be a problem. I'm working this week, give me a day or so to get my code up and we can go from there. |
Alright then, guess I'll check how is Broccoli's source mapping going for the time being. |
We have two situations, the combination of which prevent us from doing any special magic to find out the relative path between output CSS and sourcemaps.
The only solution to the sourcemap path issue I see is having the user specify the relative path from their output directory as seen by the server to their source directory as seen by the server. We can join this with some knowns to get the correct relative path. Caveats: the source files must always be visible to the web browser, under the webroot. If the files are outside of the webroot the user will need to use some connect middleware to expose them. In a Yeoman type project this would actually be easy: I need to expand the docs a little and do some more tests, but I think this is where the feature will land. It's not super easy to explain but I'm pretty sure there's no magic we can do behind the scenes without knowing both the output dir and the webroot. |
@UltCombo You were right, I found where the extra |
Yep, that makes perfect sense.
That doesn't seem to be a caveat, rather it is by design -- logically, it is impossible for the browser to succeed in requesting a file which the web server is not serving.
Oh, not only Yeoman, even with just Express it is not too uncommon to serve aliased static resources, e.g.
No problem. |
Or, I could add a |
Hm, double encoding wouldn't be great. I'm using |
If you were going to add logic to uri-path, I don't think it would need to be an option. No one wants to double encode, right? |
Oh I see. It would be hard, if not impossible, to do proper double-encoding prevention as My initials thoughts were to decode any % escape sequences and then encode the URL path -- this way, no double encoding occurs and it handles cases where only part of the URL path are properly encoded. But this would break when the file path/name actually contains the |
The two options that would work 100% for
|
OK, just some better docs and this should be good to go. |
Your logic seems pretty robust, though I'm not sure what the "original sourcemap path" actually means. I'll pull your commit and run some tests as well. |
Oh, by "original sourcemap path" you mean the
But I guess including //cc @robwierzbowski |
The sourceMappingURL contains the filenames and is written by Sass, so any explosions that happen there can be handled upstream. I think it's 👌 I'm happy with how this is working. I really appreciate the fast testing and review, BTW. |
You're welcome. But oh, guess I wasn't very clear. I meant that the The |
Gawd, I'm having a hard time to articulate statements together, I should probably sleep a bit more at night. Anyway, edited my previous comment for clarity. |
Running sass directly on a file called |
@robwierzbowski the source map spec expects properly encoded URL(s) in the sources array. Spaces cannot appear unencoded in a URL. I know this is a rare use case, but we're so close to be fully sourcemap spec compliant. |
If it's not compliant it needs to be fixed upstream so the fix bubbles down to everybody. You seem to know a ton about the sourcemap spec — want to file an issue? |
@robwierzbowski are you sure this is an upstream bug? I thought this was a bug with this plugin's source map rewriting, I'll double check it with vanilla Sass. |
@robwierzbowski yep, it is actually an upstream bug. My bad. |
That was on Mac with Sass 3.3.8. Might be different on Win, but I'm guessing not. |
@robwierzbowski ohh, I somehow skipped the "Running sass directly on a file" part, my bad. Haven't slept very well lately, guess I'll file an issue in the Sass repository after I take a nap. Your PR looks great, good job. |
No worries! |
Adds a sourcemapPath option for specifying the relative path from CSS to source files Normalizes Windows paths to Unix/uri style with slash(). Tested with many combinations in browser. 👌.
OK @sindresorhus, this is ready to go. |
@robwierzbowski awesome :) |
Hey guys, very new to Sass sourcemaps and crazy coincidence that this issue is being worked on right when I realize I'm having this problem! (I think). I'm running a Django app locally and my Sass partial imports look like this: That works fine & well for grabbing the partial and generating the compiled CSS, but my local webserver has this file available at
|
Gulp is a separate build system, not connected to django in any way. The On Saturday, July 5, 2014, Daniel Shapiro notifications@github.com wrote:
Rob Wierzbowski |
First off, source maps are in a state of chaos, to be honest. Each tool and each task runner/build tool/asset packager handles source maps differently.
Yeah, this plugin has just added a "non-standard"
See the Sass Documentation:
As you can see, these use cases are often easier to workaround in the build tool/asset packager.
I don't have experience with django-pipeline, sorry. If it is pluggable, that is, if it accepts adding 3rd-party plugins/tasks to the packaging logic, you could (in gulp terminology) pipe the maps into a task to rewrite their It is worth opening an issue in their repository, in my opinion. In the worst case, you could just watch the |
@UltCombo awesome thanks for that information! Just to reiterate/confirm my understanding: this plugin here is opening up the sourcemap file that Sass generates and it is changing the paths in the sources JSON to resolve this issue, right? i.e. that is what django-pipeline would have to do to emulate the functionality you guys are adding. |
@danxshap yes, that's the logic which @robwierzbowski has implemented to fix this issue. You can see it here: Lines 12 to 43 in 0a2359c
|
Great, really appreciate the info. Thank you and have a great rest of your weekend! |
No problem, you too. |
Tested with many combinations in browser. 👌.
Doesn't normalize Windows paths yet. Putting up for review but want to add that before merge.