Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds compatibility for gulp-sourcemaps #51

Merged
merged 5 commits into from
Sep 24, 2014

Conversation

jessepollak
Copy link
Contributor

This PR brings in the new source mapping system that's being spec'd in gulpjs/gulp#356 and floridoo/gulp-sourcemaps.

Note: until gulp-sourcemaps/gulp-sourcemaps#4 is merged, the sourceMappingURL in the output CSS files will be incorrectly formatted.

@jessepollak jessepollak changed the title Adds compatibility for gulp-sourcemap Adds compatibility for gulp-sourcemaps May 10, 2014
@jessepollak
Copy link
Contributor Author

The one nasty thing about this change is that libsass automatically appends the sourceMappingURL to the CSS file. This PR removes it so that gulp-sourcemaps can re-add it on write().

I've opened up issues for adding an option to not automatically append this comment at sass/libsass#357 and sass/node-sass#305.

@dlmanning
Copy link
Owner

Hi folks, thanks for the PR, I will evaluate and try to get this merged in soon. Seems like source maps are the root of a lot of reported issues.

@davidgilbertson
Copy link

Line 68 in README.md: .pipe(sass) should be .pipe(sass()) no?

@avanderhoorn
Copy link

+1 for this! Just wondering when this is going to come in? I'm wanting to use this in combination with gulp-autoprefixer and they are just getting their gulp-sourcemaps in place.

@ocombe
Copy link

ocombe commented Jul 19, 2014

That would be a nice addition, +1 for this !

@chiefjester
Copy link

+1 for this!

@steven-prybylynskyi
Copy link

We pipe it to gulp-autoprefixer too, without source maps yet.
Waiting for a fix. Hopefully this PR will be a cure.

@chrisspiegl
Copy link

+1

3 similar comments
@gabepaez
Copy link

gabepaez commented Aug 5, 2014

+1

@booleanbetrayal
Copy link
Contributor

👍

@barillax
Copy link

👍

@booleanbetrayal
Copy link
Contributor

looks like some minor API changes preventing this PR from working as is:

@jessepollak
Copy link
Contributor Author

This is also on me — was planning on getting to it later today (sticky on my computer). I'll take a look right now.

@dlmanning
Copy link
Owner

Will look at this again when @jessepollak is happy with his PR

@jessepollak
Copy link
Contributor Author

@booleanbetrayal I'm not sure what you mean with your first fix, but I switched to vinyl-sourcemaps-apply and things look good to me here

@jessepollak
Copy link
Contributor Author

Would be great to have a few people run it through though, I just came back to the code for the first time in a few months.

@dlmanning
Copy link
Owner

An issue from my perspective is that right now many people are using gulp-sass's inlined source maps, I'll break their builds if I just pull that feature out.

Probably I should merge this feature and take gulp-sass 1.0.

@dlmanning
Copy link
Owner

I've merged in @jessepollak's PR, along with a couple of touch-ups on https://github.com/dlmanning/gulp-sass/tree/dev. It appears to work correctly with gulp-sourcemaps to me. Would anyone else mind double checking? If everything seems good I'll update the docs and release as 1.0 tomorrow.

@jessepollak
Copy link
Contributor Author

👏 thanks @dlmanning!

@patrickmarabeas
Copy link

@dlmanning Error handling (errLogToConsole & onError) and sourcemaps (gulp-sourcemaps) aren't working at the same time - in the same manner that sourceComments broke error handling previously.

@lunelson
Copy link

I have the following configuration, using the dev branch, not throwing any errors currently though the sourcemaps aren't picking up yet either ;(

gulp.task('sass', function () {
    gulp.src('test/test.scss')
        .pipe(sourcemaps.init())
        .pipe(sass({
            errLogToConsole: false,
            onError: function(err) {
                browserSync.notify(err, 2000);
                console.log(err); }
        }))
        .pipe(autoprefixer({
            browsers: ['last 3 versions'],
            cascade: false
        }))
        .pipe(sourcemaps.write('.'))
        .pipe(gulp.dest('test'))
        .pipe(filter('*.css')) // necessary?
        .pipe(browserSync.reload({stream:true}));
});

@patrickmarabeas
Copy link

@lunelson I put sourcemaps.write above autoprefixer and it works as expected as far as I can tell...

@lunelson
Copy link

Hmm. I'm using the new gulp-autoprefixer which is now maintained by @sindresorhus. The section on sourcemaps seems to indicate that autoprefixer should come after; but it's unclear to me whether it's supposed to be parsing inline or external maps in that example...

@patrickmarabeas
Copy link

This seems to work if you want both within gulp-sourcemaps:

.pipe(sourcemaps.init())
.pipe(sass({ errLogToConsole: true }))
.pipe(gulpif(development, sourcemaps.write()))

.pipe(sourcemaps.init({ loadMaps:true }))
.pipe(prefix({
  browsers: ['last 3 versions'],
  cascade: false
}))
.pipe(gulpif(development, sourcemaps.write()))

@lunelson
Copy link

Thanks, I'll give that a try when i get back to it

@dlmanning
Copy link
Owner

1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet