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

Add options for imports and plugins #6

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
84 changes: 62 additions & 22 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,67 @@
var stylus = require('stylus');
var path = require('path');
var fs = require('fs');

var stylusUtils = require('stylus/lib/utils');

var stylusLookupIndex = stylusUtils.lookupIndex;

stylusUtils.lookupIndex = function(name, paths, filename){
var nodeModuleMatch = paths.find(includePath => includePath.endsWith(name));

var found = stylusLookupIndex(name, paths, filename);

// check for node_modules/<name>
if (!found && nodeModuleMatch) {
found = stylusLookupIndex(nodeModuleMatch, paths, filename);
}
if (!found) {
console.warn('[derby-stylus] Could not find ' + name + ' in ' + paths.join(', '));
}
return found;
};


module.exports = function(app, options = {}) {

Choose a reason for hiding this comment

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

Using default params and spread operator means this is no longer compatible with ES5, so reminder that the new minimum of ES6 (ES2015) should be called out in the release notes

var { includes = [], plugins = [], ...opts } = options;

Choose a reason for hiding this comment

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

It looks like opts is intended to be a base set of options to be passed directly to the Stylus compiler?

If that's the case, I'd prefer them to be passed as an stylusOptions property for clarity, instead of intermingled with the Derby plugin options.

Can also name the local var to emphasize that it's a base set of Stylus options, since it can be overridden later by individual loadStylesSync calls (also see comment further below)

var baseStylusOptions = options.stylusOptions;
...
function stylusCompiler(file, filename, options) {
  var options = { _imports: [], ...baseStylusOptions, ...options };
  ...


var stylusRequirePath = require.resolve('stylus');
var nodeModulesRoot = stylusRequirePath.substring(0, stylusRequirePath.indexOf('/stylus'));

Choose a reason for hiding this comment

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

Would be better to use path.dirname() here for Windows compatibility


var includePaths = includes.reduce((acc, moduleId) => {

Choose a reason for hiding this comment

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

More of a stylistic thing, I think forEach makes for easier to read code than reduce

var stylusModulePath = path.join(nodeModulesRoot, moduleId);
if (fs.existsSync(stylusModulePath)) {
acc.push(stylusModulePath);
}
return acc;
}, []);

function stylusCompiler(file, filename, options) {
var options = { _imports: [], ...options, ...opts };

Choose a reason for hiding this comment

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

The options argument here was passed in a specific loadStylesSync call:
https://github.com/derbyjs/derby/blob/bcdbeaccb0a0fc0293022ce3574d3f414bcf4f42/src/files.ts#L59-L76

So the more specific call's options should take precedence over the base options provided to the plugin as a whole.

var out = {};
var compiler = stylus(file, options)
.set('paths', includePaths)
.set('filename', filename)
.set('compress', options.compress)
.set('include css', true)

plugins.forEach(plugin => {
var pluginFn = require(plugin);
compiler.use(pluginFn());
});

compiler.render(function(err, value) {
if (err) throw err;
out.css = value;
});

out.files = options._imports.map(function(item) {
return item.path;
});
out.files.push(filename);
return out;
}

module.exports = function(app) {
app.styleExtensions.push('.styl');
app.compilers['.styl'] = stylusCompiler;
};

function stylusCompiler(file, filename, options) {
options || (options = {});
options._imports || (options._imports = []);
var out = {};
var s = stylus(file, options)
.set('filename', filename)
.set('compress', options.compress)
.set('include css', true);


s.render(function(err, value) {
if (err) throw err;
out.css = value;
});
out.files = options._imports.map(function(item) {
return item.path;
});
out.files.push(filename);
return out;
}
15 changes: 9 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"name": "derby-stylus",
"version": "0.3.0",
"name": "@derbyjs/derby-stylus",
"version": "1.0.0-beta.2",
"description": "Derby plugin to add Stylus support",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
"test": "echo \"Error: no test specified\" && exit 0"
},
"repository": {
"type": "git",
Expand All @@ -16,14 +16,17 @@
"stylus",
"plugin"
],
"author": "Artur Zayats <[email protected]>",
"contributors": [
"Artur Zayats <[email protected]>",
"Craig Beck <[email protected]>"
],
"license": "MIT",
"bugs": {
"url": "https://github.com/derbyjs/derby-stylus/issues"
},
"homepage": "https://github.com/derbyjs/derby-stylus",
"dependencies": {
"stylus": "^0.54.5"
"peerDependencies": {
"stylus": "~0.59.0"
},
"directories": {
"example": "example"
Expand Down