Skip to content
This repository was archived by the owner on Oct 2, 2020. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 4 commits
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
9 changes: 7 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"scripts": {
"doc": "esdoc -c ./esdoc.json",
"compile": "git clean -xdf lib && babel -d lib/ src/",
"prepublish": "npm run compile"
"prepublish": "npm run compile",
"test": "./node_modules/.bin/mocha test/**/* -- --harmony"
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -52,9 +53,13 @@
"babel-plugin-transform-runtime": "^6.12.0",
"babel-preset-es2016-node5": "^1.1.2",
"babel-preset-react": "^6.11.1",
"babel-register": "^6.18.0",
"chai": "^3.5.0",
"chai-as-promised": "^6.0.0",
"esdoc": "^0.4.8",
"esdoc-es7-plugin": "0.0.3",
"esdoc-plugin-async-to-sync": "^0.5.0",
"eslint": "^3.3.0"
"eslint": "^3.3.0",
"mocha": "^3.2.0"
}
}
80 changes: 76 additions & 4 deletions src/css/sass.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import path from 'path';
import fs from 'fs';
import toutSuite from 'toutsuite';

import {CompilerBase} from '../compiler-base';
Expand Down Expand Up @@ -48,15 +49,21 @@ export default class SassCompiler extends CompilerBase {

paths.unshift('.');

const { includePaths } = this.compilerOptions
if (includePaths) {
sass.importer(this.buildImporterCallback(includePaths))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good fix!

delete this.compilerOptions.includePaths;
}

let opts = Object.assign({}, this.compilerOptions, {
indentedSyntax: filePath.match(/\.sass$/i),
sourceMapRoot: filePath,
sourceMapRoot: filePath,
});

let result = await new Promise((res,rej) => {
sass.compile(sourceCode, opts, (r) => {
if (r.status !== 0) {
rej(new Error(r.formatted));
rej(new Error(r.formatted || r.message));
return;
}

Expand Down Expand Up @@ -93,9 +100,15 @@ export default class SassCompiler extends CompilerBase {

paths.unshift('.');

const { includePaths } = this.compilerOptions
if (includePaths) {
sass.importer(this.buildImporterCallback(includePaths))
delete this.compilerOptions.includePaths;
}

let opts = Object.assign({}, this.compilerOptions, {
indentedSyntax: filePath.match(/\.sass$/i),
sourceMapRoot: filePath,
sourceMapRoot: filePath,
});

let result;
Expand All @@ -114,8 +127,67 @@ export default class SassCompiler extends CompilerBase {
};
}

buildImporterCallback (includePaths) {
const self = this;
return (function (request, done) {
let file
if (request.file) {
done();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing return statement

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

} else {
// sass.js works in the '/sass/' directory
const cleanedRequestPath = request.resolved.replace(/^\/sass\//, '');
for (let includePath of includePaths) {
const filePath = path.resolve(includePath, cleanedRequestPath);
let variations = sass.getPathVariations(filePath);

file = variations
.map(self.fixWindowsPath.bind(self))
.reduce(self.importedFileReducer.bind(self), null);

if (file) {
const content = fs.readFileSync(file, { encoding: 'utf8' });
return sass.writeFile(file, content, () => {
done({ path: file })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

return here too - even though we don't need it, just get in the habit of always writing return after a callback, it will save you much sanity in the long run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

});
}
}

if (!file) done();
}
});
}

importedFileReducer(found, path) {
// Find the first variation that actually exists
if (found) return found;

try {
const stat = fs.statSync(path);
if (!stat.isFile()) return null;
return path;
} catch(e) {
return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We probably want to debug log this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this - sass.js returns ~25 variations, each invalid one will fail here and trigger a debug log. In medium-sized projects with multiple sass files, this will generate a whole lot of unnecessary output. Correct me if I'm wrong :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@saschagehlich Use the debug module, it will only log if a user set's their environment variables up explicitly to debug electron-compile

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know about the debug module, but still, this would output 25 lines of debug information for every sass file that is being compiled. Does that make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, in that case we'll just hope that Sass provides enough debugging

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If sass.js doesn't find an imported file, it reports an error anyway, so I guess that's fine

}
}

fixWindowsPath(file) {
// Unfortunately, there's a bug in sass.js that seems to ignore the different
// path separators across platforms. We need to fix this.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you give some examples of what gets passed in and what gets returned?

Copy link
Copy Markdown
Contributor Author

@saschagehlich saschagehlich Jan 17, 2017

Choose a reason for hiding this comment

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

✅ (see new commits)

if (process.platform === 'win32' && file[0] === '/') {
file = file.slice(1);
}

if (file[0] === '_') {
const parts = file.slice(1).split(path.sep);
const dir = parts.slice(0, -1).join(path.sep);
const fileName = parts.reverse()[0];
file = path.resolve(dir, '_' + fileName);
}
return file
}

getCompilerVersion() {
// NB: There is a bizarre bug in the node module system where this doesn't
// NB: There is a bizarre bug in the node module system where this doesn't
// work but only in saveConfiguration tests
//return require('@paulcbetts/node-sass/package.json').version;
return "4.1.1";
Expand Down
Empty file.
2 changes: 2 additions & 0 deletions test/css/fixtures/importable.sass
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
body, html
background: red
46 changes: 46 additions & 0 deletions test/css/sass.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import path from 'path'
import SassCompiler from '../../src/css/sass';

describe('css/sass', function () {
this.timeout(5000)
it('should correctly compile valid sass', (done) => {
const sass = 'body, html\n background: red';
const filename = 'file.sass';

const compiler = new SassCompiler();
compiler.compilerOptions = { comments: false };

compiler.compile(sass, filename)
.then((result) => {
expect(result.mimeType).to.equal('text/css');
expect(result.code).to.equal('body, html {\n background: red; }\n');
done();
})
.catch((err) => {
done(err);
});
});

describe('when passing includePaths', () => {
it('should correctly resolve import paths', (done) => {
const sass = '@import "importable"';
const filename = 'file.sass';

const compiler = new SassCompiler();
compiler.compilerOptions = Object.assign({}, compiler.compilerOptions, {
includePaths: ['test/css/fixtures'],
comments: false
});

compiler.compile(sass, filename)
.then((result) => {
expect(result.mimeType).to.equal('text/css');
expect(result.code).to.equal('body, html {\n background: red; }\n');
done();
})
.catch((err) => {
done(err);
});
})
})
});
4 changes: 4 additions & 0 deletions test/mocha.opts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
--require babel-register
--require test/support.js
--reporter spec
--recursive
4 changes: 4 additions & 0 deletions test/support.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'

global.expect = chai.expect