Closed marksmccann closed 5 years ago
@phillipluther This PR is ready for review. Just a heads up, I attempted to add the concat features with the strategy we discussed but ran into some road blocks. I decided to just temporarily remove the concat logic entirely since it was not compatible with source maps. I created #19 for us to pick up later to re-add. I figured it was better for me to move on than to get held up.
Also, removing that logic was technically a "breaking change" but since we haven't released the package yet, I didn't label it as one in the commit. Was that the right call?
@marksmccann Awesome! Taking a look now.
I'm at peace with removing the concatenation behavior. Inclusions of it was all based on theory/neatness; it's not functionality that node-sass
provides and we have a perfectly acceptable -- I'd even argue standard -- work around by just making manifest files that @import
the sheets you want to concat and passing paths to those. We can revisit once we get some usage feedback.
RE: breaking change, totally on board. It's breaking for whom? Me? You? I don't think we have to sweat breaking changes since those versions will never show up in NPM and it'd be impossible to experience a situation where going from one pre-release version to another breaks anything.
That is, one would never be able to npm install node-sass-extra@0.10
and then have npm install node-sass-extra@0.1.1
(or whatever) come in a break it. The module history starts at publish. I think that was the right call, in addition to not version bumping it at all.
Everything looks great! Informal approval.
outFile
supports the same features asoutput
but will not write to disk.sourceMap
, it will execute on the output path and use the returned path.sourceMaps
will be written to disk when provided along withoutput
.output
was moved out ofrender
and into a stand-alone utility.closes #18