Add Stylus loader #195
Add Stylus loader #195
Conversation
There was a problem hiding this comment.
Hi @mneuhaus,
That PR looks great, I have some comments though:
- Could you also update the
yarn.lockfile with the added dev dependency? - Could you add some tests in:
test/config-generator.js(e.g. to check that the loader is added to the generated config)test/WebpackConfig.js(e.g. to check that you can't callenableStylusLoaderwith a non-callback parameter)test/functional.js(e.g. to check that some stylus files give the expected css files)
| * Or pass options to the loader | ||
| * | ||
| * Encore.enableStylusLoader(function(options) { | ||
| * // https://github.com/shama/stylus-loader |
There was a problem hiding this comment.
Could you add another line with an example there so people know how to use options (using a real one)?
There was a problem hiding this comment.
to be honest, there isn't much that can be configured, i can not think of a good example off the top of my head. (https://github.com/shama/stylus-loader/blob/master/index.js#L33)
The primary reason, for putting this in encore is, that adding a css preprosessor that properly works with encore is a bit much boilerplate:
.addLoader({
test: /\.styl/,
use: [
{ loader: './node_modules/extract-text-webpack-plugin/dist/loader.js',
options: { omit: 1, remove: true } },
{ loader: 'style-loader?sourceMap' },
{ loader: 'css-loader',
options: {
minimize: false, sourceMap: true, importLoaders: 0
}
},
{ loader: 'stylus-loader'}
]
})
There was a problem hiding this comment.
sure, sounds fine, i added it :)
lib/config-generator.js
Outdated
| @@ -185,6 +186,13 @@ class ConfigGenerator { | |||
| }); | |||
| } | |||
|
|
|||
| if (this.webpackConfig.useStylusLoader) { | |||
| rules.push({ | |||
| test: /\.stylus/, | |||
There was a problem hiding this comment.
Is the .stylus file extension the recommended one? I saw .styl used on multiple pages:
There was a problem hiding this comment.
you're absolutely right, that is a typo, which i'll fix right away :)
|
i'll try to take care of those tonight :) 👍 |
mneuhaus
force-pushed
the
stylus-loader
branch
from
b443cf8
to
b80bd05
Compare
|
i added the tests and fixed the mentioned comments :) |
lib/features.js
Outdated
| @@ -26,6 +26,11 @@ const features = { | |||
| packages: ['less-loader'], | |||
| description: 'load LESS files' | |||
| }, | |||
| stylus: { | |||
| method: 'enableStylusLoader()', | |||
| packages: ['stylus-loader'], | |||
There was a problem hiding this comment.
I believe we also need stylus, correct?
|
@weaverryan there you go :) |
No description provided.