-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the PR description, it's not clear to be what the practical benefit of this would be. Can you add an example of the effects of using the new options?
var { includes = [], plugins = [], ...opts } = options; | ||
|
||
var stylusRequirePath = require.resolve('stylus'); | ||
var nodeModulesRoot = stylusRequirePath.substring(0, stylusRequirePath.indexOf('/stylus')); |
There was a problem hiding this comment.
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 stylusRequirePath = require.resolve('stylus'); | ||
var nodeModulesRoot = stylusRequirePath.substring(0, stylusRequirePath.indexOf('/stylus')); | ||
|
||
var includePaths = includes.reduce((acc, moduleId) => { |
There was a problem hiding this comment.
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
}; | ||
|
||
|
||
module.exports = function(app, options = {}) { |
There was a problem hiding this comment.
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
|
||
|
||
module.exports = function(app, options = {}) { | ||
var { includes = [], plugins = [], ...opts } = options; |
There was a problem hiding this comment.
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 };
...
}, []); | ||
|
||
function stylusCompiler(file, filename, options) { | ||
var options = { _imports: [], ...options, ...opts }; |
There was a problem hiding this comment.
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.
Move dependency on stylus to
peerDependency
and provideoptions
to provideimports
paths for stylesheet resolution fromnode_modules
, andplugins
for specifying stylus plugins to use (likenib
)Current patched version
0.1.3
still has dependency onnib
as plugin, conflicting withv0.2.0
and later versions of this plugin that removenib
as dependency. Nib, as a plugin, may not be the right choice, or may conflict with other plugins desired, so removing this dependency and providing for option to indicate plugins to be used is provided:Usage:
and stylesheets can
require
without relative pathing tonode_modules
instead of things like
../../node_modules/branded-styles