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

[bug] ensure that SassCompiler is not run on files that are not (.scss|.sass|.css) #215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 2 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ language: node_js
node_js:
# we recommend testing addons with the same minimum supported node version as Ember CLI
# so that your addon works for all apps
- "6"
- "10"
- "12"
Comment on lines +6 to +7
Copy link
Member

Choose a reason for hiding this comment

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

This is odd, was this not handled by #216 ?


sudo: false
dist: trusty
Expand All @@ -23,9 +24,6 @@ env:
matrix:
# we recommend new addons test the current and previous LTS
# as well as latest stable release (bonus points to beta/canary)
- EMBER_TRY_SCENARIO=ember-lts-2.12
- EMBER_TRY_SCENARIO=ember-lts-2.16
- EMBER_TRY_SCENARIO=ember-lts-2.18
Comment on lines -26 to -28
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated to the goal of this PR

- EMBER_TRY_SCENARIO=ember-release
- EMBER_TRY_SCENARIO=ember-beta
- EMBER_TRY_SCENARIO=ember-canary
Expand Down
135 changes: 123 additions & 12 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,115 @@
'use strict';
/* eslint-env node */
var SassCompilerFactory = require('broccoli-sass-source-maps');
var path = require('path');
var VersionChecker = require('ember-cli-version-checker');
var Funnel = require('broccoli-funnel');
var mergeTrees = require('broccoli-merge-trees');

const path = require('path');
const Filter = require('broccoli-persistent-filter');
Copy link
Member

Choose a reason for hiding this comment

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

The use of broccoli-persistent-filter here seems odd to me. As far as I can tell, this is not a 1:1 compiler (which is essentially what broccoli-persistent-filter expects to be working with).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would be a good way to get this type of caching without broccoli-persistent-filter?

const mkdirp = require('mkdirp');
const VersionChecker = require('ember-cli-version-checker');
const Funnel = require('broccoli-funnel');
const mergeTrees = require('broccoli-merge-trees');
const fs = require('fs');
const includePathSearcher = require('include-path-searcher');

class SassCompiler extends Filter {
constructor(inputTree, inputOutputMap, _options) {
let options = _options || {};
if (!options || typeof options !== 'object') {
options = { persist: true };
} else if (typeof options.persist === 'undefined') {
options.persist = true;
}

super(inputTree, options);

this.name = 'sass-compiler';
this.options = options;
this.inputTree = inputTree;
this.inputOutputMap = inputOutputMap;
this.lastBuildStart = undefined;

this.renderSassSync = options.implementation.renderSync;

this.sassOptions = {
importer: options.importer,
functions: options.functions,
indentedSyntax: options.indentedSyntax,
omitSourceMapUrl: options.omitSourceMapUrl,
outputStyle: options.outputStyle,
precision: options.precision,
sourceComments: options.sourceComments,
sourceMap: options.sourceMap,
sourceMapEmbed: options.sourceMapEmbed,
sourceMapContents: options.sourceMapContents,
sourceMapRoot: options.sourceMapRoot,
fiber: options.fiber
};
}

baseDir() {
return __dirname;
}

processString() {
if(this.inputTree._buildStart !== this.lastBuildStart) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure of any way to make sure the files related to a specific input tree don't build again. Can multiple input trees have the same _buildStart? Is there a way we can distinguish the identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanpenner would you know of a good way to distinguish the identity of the input tree to avoid building specific trees that have built before?

Copy link
Member

Choose a reason for hiding this comment

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

Why do we care? We should not be mucking with the passed in tree directly, only with this.inputPaths...

this.inputOutputMap.forEach(({ input, output}) => {
var destFile = path.join(this.outputPath, output);
var sourceMapFile = this.sassOptions.sourceMap;

if (typeof sourceMapFile !== 'string') {
sourceMapFile = destFile + '.map';
}

mkdirp.sync(path.dirname(destFile));
Copy link
Member

Choose a reason for hiding this comment

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

Speculative mkdirp.sync is somewhat expensive, oftentimes we can avoid it by using try/catch and handling ENOENT + retry..


var sassOptions = {
file: includePathSearcher.findFileSync(input, this.inputPaths),
includePaths: this.inputPaths,
outFile: destFile
};

Object.assign(sassOptions, this.sassOptions);

try {
const result = this.renderSassSync(sassOptions);
fs.writeFileSync(destFile, result.css);

if (this.sassOptions.sourceMap && !this.sassOptions.sourceMapEmbed) {
fs.writeFileSync(sourceMapFile, result.map)
}
} catch(ex) {
this.rethrowBuildError(ex);
}
});

this.lastBuildStart = this.inputTree._buildStart;
}

return '';
}

/**
* Mutates the error to include properties expected by Ember CLI.
* See https://github.com/ember-cli/ember-cli/blob/master/docs/ERRORS.md#error-object
* @param {Error} error
*/
rethrowBuildError(error) {
if (typeof error === 'string') {
throw new Error('[string exception] ' + error);
gabrielcsapo marked this conversation as resolved.
Show resolved Hide resolved
} else {
error.type = 'Sass Syntax Error';
error.message = error.formatted;
error.location = {
line: error.line,
column: error.column
};

throw error;
}
}
}

SassCompiler.prototype.extensions = ['scss', 'sass'];
SassCompiler.prototype.targetExtension = 'css';
gabrielcsapo marked this conversation as resolved.
Show resolved Hide resolved

function SASSPlugin(optionsFn) {
this.name = 'ember-cli-sass';
Expand All @@ -21,8 +126,7 @@ SASSPlugin.prototype.toTree = function(tree, inputPath, outputPath, inputOptions
include: ['app/styles/**/*'],
annotation: 'Funnel (styles)'
})];
}
else {
} else {
inputTrees = [tree];
}

Expand All @@ -46,20 +150,27 @@ SASSPlugin.prototype.toTree = function(tree, inputPath, outputPath, inputOptions
}
}

var SassCompiler = SassCompilerFactory(options.implementation);
var ext = options.extension || 'scss';
var paths = options.outputPaths;
var trees = Object.keys(paths).map(function(file) {

var inputOutputMap = Object.keys(paths).map(function(file) {
var input = path.join(inputPath, file + '.' + ext);
var output = paths[file];
return new SassCompiler(inputTrees, input, output, options);

return { input, output };
});

var trees = [
new SassCompiler(new Funnel(mergeTrees(inputTrees), {
include: ['**/*.scss', '**/*.sass', '**/*.css'],
}), inputOutputMap, options)
];

if (options.passthrough) {
trees.push(new Funnel(tree, options.passthrough));
inputTrees.push(new Funnel(tree, options.passthrough));
}

return mergeTrees(trees);
return mergeTrees(trees, { overwrite: true });
gabrielcsapo marked this conversation as resolved.
Show resolved Hide resolved
};

module.exports = {
Expand Down
Loading