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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect generated source map and incorrect import order. #39

Merged
merged 6 commits into from
Jun 4, 2017
Merged

Fix incorrect generated source map and incorrect import order. #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
22c39e8
first we make test pass due a rollback in gulp-sourcemaps
Jan 12, 2017
d2bb1e0
Fix issue where wrong source map is generated if combined with other …
Jan 13, 2017
5780d7a
Fix issue #25 here as well...too lazy to create new branch
Jan 13, 2017
9a221ee
Opps forgot to add a dependency...
Jan 13, 2017
ff45d2d
Add explicity dev dep on source-map
Jan 13, 2017
5a10757
PR review comment, also depend on gulp-clean-css version that depends…
Feb 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 47 additions & 22 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ var gutil = require('gulp-util');
var merge = require('merge');
var applySourcemap = require('vinyl-sourcemaps-apply');

var Concat = require('concat-with-sourcemaps');

var File = gutil.File;
var PluginError = gutil.PluginError;
var createSuffixFunctionFromString = function(configValue) {
Expand Down Expand Up @@ -76,8 +78,13 @@ module.exports = function(options){
if(!shouldCreateSourcemaps) return fileToAddTo;

var map = result.maps[blessOutputIndex];
map.file = fileToAddTo.relative;
map.sources = [file.relative];
map.file = path.relative(fileToAddTo.base, fileToAddTo.path);
map.sources = [path.relative(file.base, file.path)];

//gotta assign fileToAddTo a source map so applyScourceMap can merge original
//and blessed sourcemap.
fileToAddTo.sourceMap = JSON.parse(JSON.stringify(file.sourceMap));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why stringify and parse though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to make a copy of file.sourceMap so that manipulation fileToAddTo.sourceMap will not affect file.sourceMap. If we don't do a copy then somewhere a long the pipeline file.sourceMap will get corrupted, which will result in some weird mapping file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah of course :)


applySourcemap(fileToAddTo, map);

return fileToAddTo;
Expand All @@ -103,40 +110,58 @@ module.exports = function(options){
return outputBasename + options.suffix(index) + outputExtension;
};

var addImports = function(index, contents){
// only the first file should have @imports
if(!options.imports || index){
return contents;
}

var imports = '';
var addImports = function(aFile, fileNamesOfPartsToImport){
var parameters = options.cacheBuster ? '?z=' + Math.round((Math.random() * 999)) : '';
for (var i = 1; i < numberOfSplits; i++) {
imports += "@import url('" + createBlessedFileName(i) + parameters + "');\n\n";
var filePath = path.relative(aFile.base, aFile.path);
var concat = new Concat(shouldCreateSourcemaps, filePath, '\n');
for (var i = 0; i < fileNamesOfPartsToImport.length; i++) {
concat.add(null, "@import url('" + fileNamesOfPartsToImport[i] + parameters + "');\n");
}

return imports + contents;
};
concat.add(filePath, aFile.contents, JSON.stringify(aFile.sourceMap));

aFile.contents = concat.content;
if (shouldCreateSourcemaps) {
aFile.sourceMap = JSON.parse(concat.sourceMap);
}
};

var outputFiles = [];
for(var j = numberOfSplits - 1; j >= 0; j--) {
var newIndex = numberOfSplits - 1 - j;
var outputPath = newIndex
? path.resolve(path.join(outputPathStart, createBlessedFileName(newIndex)))
: outputFilePath;
var nonMasterPartFileNames = [];
for(var j = 0; j < numberOfSplits; j++) {
var oneBasedIndex = j + 1;
var isAtLastElement = oneBasedIndex === numberOfSplits;

//last element is the "master" file (the one with @import).
var outputPath = isAtLastElement
? outputFilePath
: path.resolve(path.join(outputPathStart, createBlessedFileName(oneBasedIndex)));

outputFiles[newIndex] = addSourcemap(new File({
var outFile = addSourcemap(new File({
cwd: file.cwd,
base: file.base,
path: outputPath,
contents: new Buffer(addImports(newIndex, result.data[j]))
contents: new Buffer(result.data[j])
}), j);
}

if (options.imports) {
if (isAtLastElement) {
addImports(outFile, nonMasterPartFileNames);
} else {
nonMasterPartFileNames.push(path.basename(outputPath));
}
}

outputFiles[j] = outFile;
}

//We want to stream the "master" file first then split part1, part2, part3 ... this is mainly to maintain backward
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's still the first file / start of the CSS which has the @imports yeah?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's the end of the file that has the @imports. This is so that the original unsplit CSS selectors order can be preserved if we put @imports at the top. For example lets say we have following CSS:

//original.css
.style1 {}
.style2 {}
.style3{}

which gets split into

//original-blessed1.css
.style1 {}

//original-blessed3.css
.style2 {}

//origin.css
@import(original-blessed1.css)
@import(original-blessed2.css)

.style3{}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok cool

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adam-lynch, are you going to take this PR? It seems like this will solve the major issue with the current latest version. Thank you.

//compatibility; at least for the file emitting order ...
for(var k = 0; k < numberOfSplits; k++){
stream.push(outputFiles[k]);
var fileToPush = k == 0
? outputFiles[numberOfSplits - 1]
: outputFiles[k - 1];
stream.push(fileToPush);
}
cb()
} else {
Expand Down
5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"main": "./index.js",
"dependencies": {
"bless": "^4.0.0",
"concat-with-sourcemaps": "^1.0.4",
"gulp-util": "*",
"lodash.isfunction": "^3.0.8",
"lodash.isstring": "^4.0.1",
Expand All @@ -16,16 +17,18 @@
},
"devDependencies": {
"gulp": "*",
"gulp-clean-css": "^2.3.2",
"gulp-clean-css": "2.3.2",
"gulp-concat": "^2.6.1",
"gulp-coverage": "~0.1.24",
"gulp-each": "^0.2.0",
"gulp-load-plugins": "^1.2.0",
"gulp-mocha": "*",
"gulp-rename": "^1.2.2",
"gulp-sass": "^3.1.0",
"gulp-sourcemaps": "^1.6.0",
"proxyquire": "~1.0.1",
"should": "*",
"source-map": "^0.5.6",
"stream-assert": "^2.0.3"
},
"scripts": {
Expand Down
Loading