Add support for Sass includePaths option#55
Add support for Sass includePaths option#55anaisbetts merged 6 commits intoelectron-userland:masterfrom
includePaths option#55Conversation
|
Tests are all in electron-compile, put them there instead and use |
src/css/sass.js
Outdated
|
|
||
| const { includePaths } = this.compilerOptions | ||
| if (includePaths) { | ||
| sass.importer(this.buildImporterCallback(includePaths)) |
| return (function (request, done) { | ||
| let file | ||
| if (request.file) { | ||
| done(); |
There was a problem hiding this comment.
Missing return statement
src/css/sass.js
Outdated
| if (file) { | ||
| const content = fs.readFileSync(file, { encoding: 'utf8' }); | ||
| return sass.writeFile(file, content, () => { | ||
| done({ path: file }) |
There was a problem hiding this comment.
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
| if (!stat.isFile()) return null; | ||
| return path; | ||
| } catch(e) { | ||
| return null; |
There was a problem hiding this comment.
We probably want to debug log this
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
@saschagehlich Use the debug module, it will only log if a user set's their environment variables up explicitly to debug electron-compile
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Hm, in that case we'll just hope that Sass provides enough debugging
There was a problem hiding this comment.
If sass.js doesn't find an imported file, it reports an error anyway, so I guess that's fine
src/css/sass.js
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
Can you give some examples of what gets passed in and what gets returned?
There was a problem hiding this comment.
✅ (see new commits)
|
While this PR works fine, it seems to break electron-compile's compiler-info.json.gz file, which does not include SassCompiler's compilerOptions anymore, causing the CompileCache to generate an incorrect digest when resolving a file from a packaged application. No idea where this comes from though. Any ideas @paulcbetts ? |
|
@saschagehlich I'll look into it |
|
Imma fix this up tomorrow, sorry it's taking foreverrrrr |
|
Hey @saschagehlich, sorry this took forever. Sass actually already had an option called |
|
@paulcbetts Cool, thanks! Were you able to reproduce the issue I mentioned above? |
|
No, I tested it with an electron-forge demo app and it all worked (though I didn't try setting an explicit |
|
@paulcbetts I just started using electron-compile in a project again and it seems that |
|
Exactly the |
|
Fixed in #75 |
4 participants
This allows you to
@importfiles from specifiedincludePathsin your .sass / .scss files. I also added a test suite (mocha) and a test for this feature.A little description on what this actually does:
If an
includePathsoption is specified, the sass compiler callsSass.importerand passes an importer method that correctly resolves the import requests by iterating over the include paths and resolving the request to the correct file name. It then writes the file content to emscripten's memory usingSass.writeFileand asks the sass compiler to use the specified path for the import.