-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
#232 Added support for the eslint-loader #243
Changes from all commits
ce8c66d
a19414e
03bc596
87e1a02
0738691
22fcb14
27b641e
d06a0b3
95af361
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
const a = 'foobar'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ const tsLoaderUtil = require('./loaders/typescript'); | |
const coffeeScriptLoaderUtil = require('./loaders/coffee-script'); | ||
const vueLoaderUtil = require('./loaders/vue'); | ||
const handlebarsLoaderUtil = require('./loaders/handlebars'); | ||
const eslintLoaderUtil = require('./loaders/eslint'); | ||
// plugins utils | ||
const extractTextPluginUtil = require('./plugins/extract-text'); | ||
const deleteUnusedEntriesPluginUtil = require('./plugins/delete-unused-entries'); | ||
|
@@ -228,6 +229,16 @@ class ConfigGenerator { | |
}); | ||
} | ||
|
||
if (this.webpackConfig.useEslintLoader) { | ||
rules.push({ | ||
test: /\.jsx?$/, | ||
loader: 'eslint-loader', | ||
exclude: /node_modules/, | ||
enforce: 'pre', | ||
options: eslintLoaderUtil.getOptions(this.webpackConfig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
}); | ||
} | ||
|
||
if (this.webpackConfig.useTypeScriptLoader) { | ||
rules.push({ | ||
test: /\.tsx?$/, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* This file is part of the Symfony Webpack Encore package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
const loaderFeatures = require('../features'); | ||
const applyOptionsCallback = require('../utils/apply-options-callback'); | ||
|
||
/** | ||
* @param {WebpackConfig} webpackConfig | ||
* @return {Object} of options to use for eslint-loader options. | ||
*/ | ||
module.exports = { | ||
getOptions(webpackConfig) { | ||
loaderFeatures.ensurePackagesExist('eslint'); | ||
|
||
const eslintLoaderOptions = { | ||
cache: true, | ||
parser: 'babel-eslint', | ||
emitWarning: true | ||
}; | ||
|
||
return applyOptionsCallback(webpackConfig.eslintLoaderOptionsCallback, eslintLoaderOptions); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/* | ||
* This file is part of the Symfony Webpack Encore package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
const expect = require('chai').expect; | ||
const WebpackConfig = require('../../lib/WebpackConfig'); | ||
const RuntimeConfig = require('../../lib/config/RuntimeConfig'); | ||
const eslintLoader = require('../../lib/loaders/eslint'); | ||
|
||
function createConfig() { | ||
const runtimeConfig = new RuntimeConfig(); | ||
runtimeConfig.context = __dirname; | ||
runtimeConfig.babelRcFileExists = false; | ||
|
||
return new WebpackConfig(runtimeConfig); | ||
} | ||
|
||
describe('loaders/eslint', () => { | ||
it('getOptions() full usage', () => { | ||
const config = createConfig(); | ||
config.enableEslintLoader(); | ||
const actualOptions = eslintLoader.getOptions(config); | ||
|
||
expect(actualOptions).to.deep.equal({ | ||
cache: true, | ||
parser: 'babel-eslint', | ||
emitWarning: true | ||
}); | ||
}); | ||
|
||
it('getOptions() with extra options', () => { | ||
const config = createConfig(); | ||
config.enableEslintLoader((options) => { | ||
options.extends = 'airbnb'; | ||
}); | ||
|
||
const actualOptions = eslintLoader.getOptions(config); | ||
|
||
expect(actualOptions).to.deep.equal({ | ||
cache: true, | ||
parser: 'babel-eslint', | ||
emitWarning: true, | ||
extends: 'airbnb' | ||
}); | ||
}); | ||
|
||
it('getOptions() with an overridden option', () => { | ||
const config = createConfig(); | ||
config.enableEslintLoader((options) => { | ||
options.emitWarning = false; | ||
}); | ||
|
||
const actualOptions = eslintLoader.getOptions(config); | ||
|
||
expect(actualOptions).to.deep.equal({ | ||
cache: true, | ||
parser: 'babel-eslint', | ||
emitWarning: false | ||
}); | ||
}); | ||
|
||
it('getOptions() with a callback that returns an object', () => { | ||
const config = createConfig(); | ||
config.enableEslintLoader((options) => { | ||
options.custom_option = 'foo'; | ||
|
||
return { foo: true }; | ||
}); | ||
|
||
const actualOptions = eslintLoader.getOptions(config); | ||
expect(actualOptions).to.deep.equals({ foo: true }); | ||
}); | ||
}); |
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.
Bah. So, this looks really great. But so far, we've avoided allowing users to pass in options like this... because of potential merging issues between their config and our config. I really can't think of an issue here - their config wins, should be pretty straightforward. But it breaks from our convention. @Lyrkan what do you think?
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.
Well, the main issue I have with it is that we don't do the same thing for other methods...
It isn't a really big issue since in this you can also use a callback but it could confuse people.
I'd say that we have to decide whether or not we'll implement the same kind of trick (and also always do
Object.assign(defaultConfig, userConfig)
) to other methods.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.
Like lyrkan said, we should probably aggree on a common interface to configure the different plugins
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.
Ok, let's keep it. And then we can think about adding this on a case-by-case basis. I think sometimes it's not a good idea, because we'll need to merge the config. But in this case, even though we're merging the config, the config is flat and simple. There may be some cases where the merging could be more complex, and we could possibly not allow this then :)
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.
I can also remove this "nifty" merging and create a follow up ticket where we can further discuss/address. This to avoid breaking backward capabilities.
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.
As long as @Lyrkan agrees with the idea of starting to add this config where it makes sense, I'm cool with keeping it here for the new method. It won't break BC - it's just a "new philosophy" for us
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.
well, the callback API now support both editing the config to add more settings, and returning a new object. I'm not sure this automatic merging brings much value here