Skip to content

adds sourcemap support #16

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

Merged
merged 10 commits into from
Aug 20, 2015
Merged

adds sourcemap support #16

merged 10 commits into from
Aug 20, 2015

Conversation

madhums
Copy link

@madhums madhums commented Aug 17, 2015

I followed the gulp-concat plugin and added support for sourcemaps.
I had to add concat-with-sourcemaps as a dep.

Please review and let me know if anything has to be changed.

@madhums madhums mentioned this pull request Aug 17, 2015
@yocontra
Copy link
Member

This is great but it won't work with other sourcemaps in the pipeline. Check out how gulp-concat does it - you need to use applySourceMaps

@tracker1
Copy link
Member

@madhums I'd suggest following @contra's advice on this.. beyond that, it looks like the test are failing. Please add/update the tests accordingly.

@madhums
Copy link
Author

madhums commented Aug 18, 2015

@contra thanks, I will do it tonight. @tracker1 some tests are failing in the newly released iojs 3.0.0. It works well in the rest, but I will fix them.

@madhums
Copy link
Author

madhums commented Aug 18, 2015

@contra I added the Flush function where the sourcemapped content is pushed. I was thinking that concat-with-sourcemaps would do the applySourceMaps stuff (as this is how it is done in gulp-concat).

Let me know if this seems right. If not, should I use an ast parser and then applySourceMaps?

I tested the above in the following task, it seems to work fine.

var header = require('gulp-header');

return gulp.src(files)
  .pipe(sourcemaps.init())
    .pipe(concat('app.js'))
    .pipe(header('window.locales = { en: {} }'))
    .pipe(gulpif(minify, uglify({ mangle: false })))
    .pipe(header(banner, { pkg: pkg }))
  .pipe(sourcemaps.write('./'))
  .pipe(gulp.dest(dist + '/'));

@yocontra
Copy link
Member

Ah yeah, concat-with-sourcemaps does handle that - my bad. LGTM

@madhums
Copy link
Author

madhums commented Aug 18, 2015

Ok great! @tracker1 can you review the PR?

@tracker1
Copy link
Member

LGTM...

tracker1 added a commit that referenced this pull request Aug 20, 2015
@tracker1 tracker1 merged commit a422dd8 into gulp-community:master Aug 20, 2015
@tracker1
Copy link
Member

closes #7

@tracker1
Copy link
Member

@madhums Thanks for your great work on this.

@madhums
Copy link
Author

madhums commented Aug 20, 2015

👍

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.

4 participants