-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fix incorrect generated source map and incorrect import order. #39
Conversation
|
||
//gotta assign fileToAddTo a source map so applyScourceMap can merge original | ||
//and blessed sourcemap. | ||
fileToAddTo.sourceMap = JSON.parse(JSON.stringify(file.sourceMap)); |
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.
Why stringify and parse though?
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.
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.
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.
Ah yeah of course :)
index.js
Outdated
|
||
//We want to stream the "master" file first then split part1, part2, part3 ... this is mainly to maintain backward | ||
//compatibility; at lease for the file emitting order ... |
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.
least*
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.
Ah, thanks for catching it I will fix it.
test/main.js
Outdated
fs.readFileSync('./test/css/long-split.map').toString('utf8') | ||
); | ||
|
||
var smConsumer = new SourceMapConsumer(result.sourceMap); |
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.
I think you should use full name for variables 👍
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.
sure I will do that :)
test/main.js
Outdated
sourceMapPart1.version.should.equal(3); | ||
sourceMapPart1.file.should.equal("../" + concatName); | ||
sourceMapPart1.sources.should.deepEqual([sourceName]) | ||
//yes, souce content should be from the small.css which included by parente.scss |
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.
You mean long-parent.scss
right?
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.
Actually no, I meant small.css
because long-parent.scss
just contains lots of @import
of small.css
so technically stuff in small.css
should be the source for everything in the blessed file; stuff in long-parent.scss
is just container of the actual source.
And I will fix the the typo in "parente.scss"...
index.js
Outdated
var nonMasterPartFileNames = []; | ||
for(var j = 0; j < numberOfSplits; j++) { | ||
var oneBasedIndex = j + 1; | ||
var isAtLastElement = j == numberOfSplits - 1; |
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.
This is nitpicking but var isAtLastElement = oneBasedIndex === numberOfSplits;
is clearer
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.
for sure, I will update this.
|
||
//We want to stream the "master" file first then split part1, part2, part3 ... this is mainly to maintain backward |
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.
So it's still the first file / start of the CSS which has the @imports yeah?
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.
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{}
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.
Ah ok cool
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.
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.
… on clean-css that is < 4.0 (as they change too much stuff in 4.0 and makes our test unstable, will need to dig into that as separate task
4.0.0 released. Thanks a million! |
Fix issue #38 and #25.
This change probably will break backward compatibility, so I plan to do a major version bump release.