#232 Added support for the eslint-loader #243

pinoniq · 2018-01-18T11:40:47Z

I wasn't sure about whether to include the eslint-loader in the devDependencies or dependencies.

Lyrkan

Looks like a great PR overall, I only have some minor comments for now :)

I think that you made the right choice by putting eslint-loader in the dev dependencies since it isn't enabled by default and won't be needed in some case (for instance if a project only use TypeScript).

Maybe also adding a small functional test would be a good idea to check that eslint works properly.

@weaverryan WDYT about using parameters like eslintLoaderOptionsOrCallback that can either be a string, an object or a callback? Maybe the same thing could be done for some existing enableXxxxxLoader() methods?

Lyrkan · 2018-01-18T12:59:23Z

index.js

@@ -288,9 +288,11 @@ const publicApi = {
     * than the DefinePlugin:
     *
     *      const Encore = require('@symfony/webpack-encore');
-     *      const PluginPriorities = require('@symfony/webpack-encore/lib/plugins/plugin-priorities.js');
+     *      const PluginPriorities =
+     * require('@symfony/webpack-encore/lib/plugins/plugin-priorities.js');


Not really related to that PR.
I'm not against avoiding long lines but in this case I find it less clear with the line break.

it's Jetrbains :p I'll remove it

Lyrkan · 2018-01-18T12:59:49Z

index.js

     *
-     *      Encore.addPlugin(new MyWebpackPlugin(), PluginPriorities.DefinePlugin);
+     *      Encore.addPlugin(new MyWebpackPlugin(),
+     * PluginPriorities.DefinePlugin);


Same comment for that line break.

again: it's Jetrbains :p I'll remove it

Lyrkan · 2018-01-18T13:00:41Z

index.js

+               emitWarning: false
+     *     });
+     *
+     *     // For a mroe advanced usage you can pass in a callback


mroe => more

Lyrkan · 2018-01-18T13:01:18Z

index.js

+     *     // https://github.com/MoOx/eslint-loader#options
+     *     Encore.enableEslint((options) => {
+     *          options.extends = 'airbnb';
+     *          options.emitWarning = fasle;


fasle => false

Lyrkan · 2018-01-18T13:03:23Z

lib/features.js

+        method: 'enableEslintLoader()',
+        // eslint is needed so the end-user can do things
+        packages: ['eslint', 'eslint-loader', 'babel-eslint', 'eslint-plugin-import'],
+        description: 'load VUE files'


"Enable ESLint checks" ?

obvious copy paste :)

Lyrkan · 2018-01-18T13:05:05Z

lib/loaders/eslint.js

+            parser: 'babel-eslint',
+            emitWarning: true,
+            rules: {
+                'linebreak-style': 'off'


Should we really disable that rule by default?

Given most people on windows have git doing a "checkout crlf, commit lf", I feel it's an option that should be default everywhere.

I'm not sure that should be Git's job to do that kind of thing, especially since that's not its default behavior on Windows (you have to switch the core.autocrlf manually).

A cleaner way to handle that IMO is to use an .editorconfig file (which is supported by many editors) or change manually the config of your IDE so it doesn't use CRLF line breaks.

Note that this rule is also enabled in Airbnb and Google ESLint presets:

https://github.com/airbnb/javascript/blob/69a3554100935b79b9c444102504f98103834079/packages/eslint-config-airbnb-base/rules/style.js#L158-L160

https://github.com/google/eslint-config-google/blob/394bf3c9f858b83514fdcbf23d74492e253d611f/index.js#L214

Hmm, true. With default, I was referring to tools like the github desktop client and Atlassian sourcetree. I'll remove this config :)

Lyrkan · 2018-01-18T13:16:49Z

index.js

+     * https://github.com/MoOx/eslint-loader
+     *
+     *     // enables the eslint loaded using the default eslint configuration.
+     *     Encore.enableEslint();


enableEslint => enableEslintLoader

Lyrkan · 2018-01-18T13:16:57Z

index.js

+     *     Encore.enableEslint();
+     *
+     *     // Optionally, you can pass in the configuration eslint should extend.
+     *     Encore.enableEslint('airbnb');


enableEslint => enableEslintLoader

Lyrkan · 2018-01-18T13:17:05Z

index.js

+     *
+     *     // You can also pass in an object of options
+     *     // that will be passed on to the eslint-loader
+     *     Encore.enableEslint({


enableEslint => enableEslintLoader

Lyrkan · 2018-01-18T13:17:20Z

index.js

+     *
+     *     // For a mroe advanced usage you can pass in a callback
+     *     // https://github.com/MoOx/eslint-loader#options
+     *     Encore.enableEslint((options) => {


enableEslint => enableEslintLoader

pinoniq · 2018-01-18T13:24:02Z

@Lyrkan I think it's probably good to have a follow up ticket to have a common interface for all enableXXX() methods. This would allow us to abstract a part away into a helper method that "automaigcally" transforms the passed in parameter(s) into a optionsCallback.

pinoniq · 2018-01-18T15:14:08Z

Hey, concerning the functional test. Not sure how to continue here since I would not be testing creation of files, but console output. If you point into the right direction I'm more than willing to look into it

Lyrkan · 2018-01-18T16:13:56Z

@pinoniq You could do something like that: https://gist.github.com/Lyrkan/94aa818b11b35715857a52471d43e6de

The idea being to check the stats object returned by Webpack (we already do that in the "Vue.js error when using non-activated loader" test).

Some things you have to be careful though:

you need to force eslint-loader to emit errors instead of warning (you could technically use stats.toJson().warnings but warnings would pollute the tests results since we display them here)
be aware that the .eslintrc.js file at the root of Encore will be used and that if we change something in it later your test may break. To avoid that you can force the rules you'll use in your checks when calling enableEslintLoader

Another way to test that would be to emit a report (using the outputReport option) and check it once webpack is done processing everything.

pinoniq · 2018-01-25T10:18:41Z

@Lyrkan I added the test (by using your gist). Concerning the warnings/errors kind of complicates a "proper" functional test. This because eslint allows you to define errors and warnings.

It is possible to easy test this, but it pollutes the test output (due to the warnings being displayed during test results like you pointed out). What do you prefer here? Only testing for the errors output, or also adding the test for warnings and errors? If the later, maybe look into adding a flag to runWebpack to not output warnings?
e.g.
function runWebpack(webpackConfig, callback, allowCompilationError = false, logWarnings = true) {

Lyrkan · 2018-01-25T13:09:21Z

👍

@pinoniq I think the test using errors should be enough... we only need to check if ESlint is enabled and takes our conf into account, not how it behaves if we use other settings (that's already part of the ESlint test suite)

pinoniq · 2018-01-25T13:16:50Z

Ok. valid point. You want me to squash the commits into a single one? Or how do you guys do this here?

weaverryan · 2018-02-02T15:04:34Z

No need to squash - we can handle that. But thanks!

weaverryan

Sorry for the slow review! This is really great work - very high quality. I have a few questions so that I understand how this works :).

Cheers!

weaverryan · 2018-02-10T23:16:13Z

lib/loaders/eslint.js

+                webpack: {
+                    config: 'webpack.config.js'
+                }
+            }


Can you explain this config? What does this do exactly?

because you can define additional alias in webpack, this import/Resolver allows eslint to verify that libraries/files exist.

So, with config: 'webpack.config.js', we are telling eslint that it should internally require our webpack.config.js file to learn about the aliases? If so, I think that would cause an error - as the Encore "runtime" config wouldn't be defined. Are we sure this works? If we can add a test case for it in functional.js, I would be SUPER happy :)

Ping! This is my main concern / question about this PR. I simply don't understand what this config does, so I'm uncomfortable merging. Can you guide me @pinoniq?

I did some tests on a project where we use this import/resolver for webpack. And it does work with this PR. However, i'm not sure why/how.

However, I feel we should remove this lines given it's the default value of this resolver is used:
https://www.npmjs.com/package/eslint-import-resolver-webpack

Will look for webpack.config.js as a sibling of the first ancestral package.json, or a config parameter may be provided with another filename/path either relative to the package.json, or a complete, absolute path.

I also tried writing a test, but this is rather difficult since there is no webpack.config available durint test runs. Given I however propose to remove the comment above, I can remove all references to this module. This way, if you want to use it, you can simply add it to the your personal project. IF you need to chagne the config, you can using the config object you pass in. wdyt?

Yes! I think this is exactly what we should do. So, once you've removed this config and the reference to the package, I think this is ready :).

weaverryan · 2018-02-10T23:19:01Z

lib/loaders/eslint.js

+        loaderFeatures.ensurePackagesExist('eslint');
+
+        const eslintLoaderOptions = {
+            parser: 'babel-eslint',


According to their docs:

You only need to use babel-eslint if you are using types (Flow) or experimental features not supported in ESLint itself yet. Otherwise try the default parser (you don't have to use it just because you are using Babel).

I don't think this applies to Encore users with "standard" config. If they're using experimental features, then they would need to configure some extra babel presets or plugins (which is something custom). So, I'm wondering if there's a good enough reason to use this parser.

No it doesnt. I should probably check here if enableBable or enableRract thingies are called and add by default. @Lyrkan @weaverryan is there a sane way of doing this?

Hmm. Yea.... now that I process you reply, I do think it should be added always :p. The reason is that there is NOT an enableBabel() method - it's always enabled. And so, even though most users won't use any special syntax, we should try to make lives easier in case they do. If we did allow a user to disable babel in the future, then we could backup to the normal linter.

Ok, so I leave it in now?

weaverryan · 2018-02-10T23:20:13Z

lib/features.js

+    eslint: {
+        method: 'enableEslintLoader()',
+        // eslint is needed so the end-user can do things
+        packages: ['eslint', 'eslint-loader', 'babel-eslint', 'eslint-plugin-import'],


I don't quite understand what the eslint-plugin-import is supposed to do - obviously something related to parsing imports, etc :). Would it be possible to enhance the fixtures/eslint.js file to take advantage of a feature supplied by this library, so we can make sure it's configured correctly?

given this is only useful if you actually use es2015+ import syntax, maybe even just remove it, no?

If we remove it, and someone does use import, what happens? What do we lose? Do we just lose some feature of eslint that validates that those paths are correct? Or is it something different?

Just re-read the docs. if you don't use the import sytnac, but e.g. a commonJS or AMD syntax, then this will throw errors about this.
Personally, I prefer going for "enable by default" since this is my personal (and companies) use-case. However i'm not sure if we can simply enforce it. It should be easily disabled/enabled atleast.

Maybe look into having a way of advising users to install a package? or checking at run-time if it's installed or not?

Hmm. Let me see if I understand this correctly:

A) Without eslint-plugin-import, any import statements aren't validated, so in theory, you could be importing a non-existent path and would go no lint error. Are normal require paths linted?

B) *With( eslint-plugin-import, import statements are validated, but now you MUST use import in all situations: any calls to require will fail.

Is this correct? Actually, when I'm reading, I think (B) might be incorrect. I see the rule you're talking about - eslint-plugin-import. But under Installation, it talks about how all the rules are disabled by default.

If we're not sure, then "less" and more "default" setup is better. The most important thing is that we're creating a really easy "system" for enabling eslint AND for adding your own plugins.

See my comment in the other track, I think it's best to remove this required dependency.

FYI, require still works. This plugin just analyses your import statements.

I’ll take a closer look at your updates and comments soon, but yea, let’s remove this :).

Thanks for your work!

@weaverryan Anything left for me on this MR?

weaverryan · 2018-02-10T23:21:28Z

index.js

+     *     Encore.enableEslintLoader({
+     *         extends: 'airbnb',
+               emitWarning: false
+     *     });


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?

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.

Like lyrkan said, we should probably aggree on a common interface to configure the different plugins

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 :)

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.

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

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

weaverryan · 2018-02-10T23:23:59Z

lib/config-generator.js

+                loader: 'eslint-loader',
+                exclude: /node_modules/,
+                enforce: 'pre',
+                options: eslintLoaderUtil.getOptions(this.webpackConfig)


Should we use cache: true?

Can you add the cache: true?

weaverryan · 2018-03-31T17:39:42Z

ping @pinoniq! This is basically done (or maybe completely done), but I had a few pending questions/concerns that I need your help answering :)

pinoniq · 2018-05-07T14:14:45Z

@Lyrkan @weaverryan I believe all issues have been corrected in the meantime. Are there still some blocking issues left?

you want me to resolve the conflicts for you? or will you be doing that doing the merge?

Corrected spelling mistakes Reverted unrelated changes

Lyrkan · 2018-05-08T07:59:37Z

@pinoniq Sorry for the delay, I just rebased it and:

fixed a line-endings issue in your last commit (it looks like your IDE switched to CRLF for that one)
used the apply-options-callback method added in Allow to replace options in configuration callbacks #300
changed the index.js file a bit since we're now using a class (see Updated Encore object to class for proxy typehinting #263)
fixed the cache: true that wasn't added at the right place and caused the tests to fail

However I think that comment from @weaverryan hasn't been taken into account yet (since there still is a reference to the webpack.config.js file), could you take a look at it?

pinoniq · 2018-05-11T11:43:58Z

@Lyrkan Thx for the rebase. I also removed that reference of the import/resolver default config. Completly forgot about it. It should be ok now.

…ntLoader()

Lyrkan · 2018-05-16T16:54:59Z

I fixed some test cases that were broken due to the removal of the import/resolver conf and changed the packages check related to enableEslintLoader() so it doesn't include eslint-plugin-import anymore.

LGTM now (ping @weaverryan)

weaverryan · 2018-05-23T18:57:10Z

Thank you for your hard work and patience @pinoniq! This one is now merged :)

This PR was merged into the master branch. Discussion ---------- #232 Added support for the eslint-loader I wasn't sure about whether to include the eslint-loader in the devDependencies or dependencies. Commits ------- 95af361 Remove eslint-plugin-import from required dependencies for enableEslintLoader() d06a0b3 Fix eslint-loader test cases 27b641e Removed the default import/resolver config 22fcb14 Fix misplaced cache option for the eslint-loader 0738691 Use the apply-options-callback method for the eslint-loader 87e1a02 Added the cache configuration for the eslint-loader 03bc596 Added a small functional test for eslint output a19414e Removed a default eslint rule Corrected spelling mistakes Reverted unrelated changes ce8c66d #232 Added support for the eslint-loader

lyrixx · 2019-02-24T20:03:09Z

Hello. It would be nice to document this PR. I see in the config (by reading the code) that I could enable eslint, but I can not make it work.

bash-4.4$ yarn encore prod
yarn run v1.12.3
$ /home/app/app/offithings/node_modules/.bin/encore prod
Running webpack ...

 ERROR  Failed to compile with 1 errors                                                                                                                                             8:01:13 PM

Module build failed (from ./node_modules/eslint-loader/index.js):
Error: No ESLint configuration found.
    at Config.getLocalConfigHierarchy (/home/app/app/offithings/node_modules/eslint/lib/config.js:268:39)
    at Config.getConfigHierarchy (/home/app/app/offithings/node_modules/eslint/lib/config.js:192:43)
    at Config.getConfigVector (/home/app/app/offithings/node_modules/eslint/lib/config.js:299:21)
    at Config.getConfig (/home/app/app/offithings/node_modules/eslint/lib/config.js:342:29)
    at processText (/home/app/app/offithings/node_modules/eslint/lib/cli-engine.js:181:33)
    at CLIEngine.executeOnText (/home/app/app/offithings/node_modules/eslint/lib/cli-engine.js:690:40)
    at lint (/home/app/app/offithings/node_modules/eslint-loader/index.js:218:17)
    at transform (/home/app/app/offithings/node_modules/eslint-loader/index.js:201:18)
    at /home/app/app/offithings/node_modules/loader-fs-cache/index.js:127:18
    at ReadFileContext.callback (/home/app/app/offithings/node_modules/loader-fs-cache/index.js:31:14)
    at FSReqWrap.readFileAfterOpen [as oncomplete] (fs.js:238:13)

You may use special comments to disable some warnings.
Use // eslint-disable-next-line to ignore the next line.
Use /* eslint-disable */ to ignore all warnings in a file.
Entrypoint app = runtime.fa8f03f5.js app.5b7010d6.js
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Should I create a configuration ? I'm a bit confused

Thanks

Lyrkan · 2019-02-24T20:47:21Z

@lyrixx Yep, you have to define ESLint rules using one of the formats listed there: https://eslint.org/docs/user-guide/configuring#configuration-file-formats

Lyrkan requested changes Jan 18, 2018

View reviewed changes

Lyrkan added the Feature New Feature label Jan 18, 2018

Lyrkan approved these changes Jan 25, 2018

View reviewed changes

weaverryan requested changes Feb 10, 2018

View reviewed changes

Jeroen Meeus and others added 5 commits May 8, 2018 09:24

symfony#232 Added support for the eslint-loader

ce8c66d

Removed a default eslint rule

a19414e

Corrected spelling mistakes Reverted unrelated changes

Added a small functional test for eslint output

03bc596

Added the cache configuration for the eslint-loader

87e1a02

Use the apply-options-callback method for the eslint-loader

0738691

Lyrkan force-pushed the master branch from 663534e to 0738691 Compare May 8, 2018 07:39

Fix misplaced cache option for the eslint-loader

22fcb14

Removed the default import/resolver config

27b641e

Lyrkan added 2 commits May 16, 2018 18:46

Fix eslint-loader test cases

d06a0b3

Remove eslint-plugin-import from required dependencies for enableEsli…

95af361

…ntLoader()

weaverryan merged commit 95af361 into symfony:master May 23, 2018

#232 Added support for the eslint-loader #243

#232 Added support for the eslint-loader #243

Conversation

pinoniq commented Jan 18, 2018

Lyrkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinoniq commented Jan 18, 2018

pinoniq commented Jan 18, 2018

Lyrkan commented Jan 18, 2018

pinoniq commented Jan 25, 2018

Lyrkan commented Jan 25, 2018

pinoniq commented Jan 25, 2018

weaverryan commented Feb 2, 2018

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinoniq Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Mar 31, 2018

pinoniq commented May 7, 2018

Lyrkan commented May 8, 2018

pinoniq commented May 11, 2018

Lyrkan commented May 16, 2018 • edited Loading

weaverryan commented May 23, 2018

lyrixx commented Feb 24, 2019

Lyrkan commented Feb 24, 2019

pinoniq Feb 12, 2018 •

edited

Loading

Lyrkan commented May 16, 2018 •

edited

Loading