add ESLint as a default Brackets extension

[zaggino]

ref: #11984

mostly cut out from my extension https://github.com/zaggino/brackets-eslint

[zaggino]

@petetnt @ficristo @abose I'll need help here with testing, right now I'm using this branch as my primary and everything seems to work fine for me but that might not be the case for all types of projects.

[abose]

Instead of doing this as part of the build process, can the node module be directly packaged or submoduled to?
Right now, Cloning the git repo makes a working copy of brackets. This will mean that we have to do a npm install for brackets to work right?

[zaggino]

Yeah, we could submodule it. Shouldn't be a problem.

[zaggino]

Wait, it might be a problem because ESLint has a lot of dependencies which are not present in its repository.

[abose]

We would anyway pull all the dependencies when building, so using prepackaged node modules or submoduled with dependencies in source would have the same effect. Maybe we could trim all the dev dependencies from eslint?

[zaggino]

Yeah, I'm not exactly sure what you mean here...

[zaggino]

ESLint has a lot of runtime dependencies like espree or escope (and more). Some of those dependencies might be written in ES6 and compiled to ES5 only when published to npm - that's why submoduling doesn't make much sense here. Only thing that comes to my mind is creating a repo for installed ESLint and submoduling that, but this is quite weird. What's the problem with npm?

[abose]

I also meant NPM, But for eslint, could remove dev/testing dependencies from package.json if the install size is too big.
It is possible to put the node_modules folder in the extension directory right?

[zaggino]

Yes, it's possible to move the node_modules to the extension directory, do you want it to be committed there?

[abose]

Since we would have to ship the node modules anyway, it would be ok to put it in the extension directory. Also will avoid the NPM setups needed otherwise.

[petetnt]

I've been using this branch today and most things seem to work as expected. However, I am using eslint-plugin-react and when I launch Brackets, I get the following error:

Provider ESLint (async) failed: Error: Cannot find module 'eslint-plugin-react'

Despite this error, eslint and eslint-plugin-react work as expected after dismissing the error (by, for example, saving a .jsx file)

[zaggino]

@petetnt 634160c should fix that issue.

[petetnt]

634160c fixed it, thanks!

[petetnt]

This method could be simplified a bit by using fs.access wrapped in a promise

        configPath = projectRoot + '.eslintrc';
        fs.access(configPath, function (err) {
            if (err || !fs.statSync(configPath).isFile()) {
                opts.rules = require('eslint/conf/eslint.json').rules;
            }
        });

     /**/
        rulesDirPath = projectRoot + '.eslintrules';
        fs.access(rulesDirPath, function (err) {
            if (!err && fs.statSync(rulesDirPath).isDirectory()) {
                opts.rulePaths = [rulesDirPath];
            }
        });
    /**/
        ignorePath = projectRoot + '.eslintignore';
        fs.access(ignorePath, function (err) {
            if (!err && fs.statSync(ignorePath).isFile()) {
                opts.ignore = true;
                opts.ignorePath = ignorePath;
            }
        });

This way there's no need for noop either. The fs.accessSync can be passed an optional mode value too if one just wants to make sure that the file exists and is readable (the default is "file is visible", basically "file exists")

fs.accessSync("foo", fs.F_OK, function (err) {/*...*/});

[zaggino]

node in brackets is 0.10, no fs.access there: https://nodejs.org/docs/latest-v0.10.x/api/fs.html

[zaggino]

btw you're using accessSync and a callback function? weird pattern

[petetnt]

@zaggino Figures, I remembered that Brackets was running on 0.12. My bad 🐹

[petetnt]

@zaggino hah, wow, I wrote the thing in as async with a promise, noticed that it probably should be sync instead and then I don't know what happened. Waaaay too little coffee for morning coding. ☕

[ficristo]

ESLint supports multiple ways to define en .eslintrc file:
http://eslint.org/docs/user-guide/configuring#configuration-file-formats
Does this PR supports all of them?

Style nit: I guess it should indented by 4 spaces.

[zaggino]

@ficristo made a fix in 7917a2d to consider all config file names

@ficristo fixed indentation to 4 spaces in d9faa73

@abose against my better judgement, committed the actual ESLint into the extension folder so npm install is not required after cloning the repository ...

let me guys know if you have any more issues with this

[zaggino]

btw let me say again, we should really use npm install and not commit this to git, I get this error when switching branches on windows:

$ git checkout zaggino/eslint
fatal: cannot create directory at 'src/extensions/default/ESLint/node_modules/eslint/node_modules/handlebars/node_modules/uglify-js/node_modules/yargs/node_modules/cliui/node_modules/center-align/node_modules/align-text/node_modules/kind-of/node_modules/is-buffer/test': Filename too long

[zaggino]

had to remove ESLint's code because of the issue above with long file names ... we should just use npm install

[zaggino]

added this with npm3, no longer have the long file path problem ... node_modules occupy about 10MB of disk space this way ... that's 10MB more when cloning a repo which could be avoided by using npm

[ficristo]

As shown by travis grunt eslint doesn't pass.
If I click on an error the cursor goes on the right row but on the wrong colum. (Should go on the previous one)
The dependencies in node_modules should be tracked in the package.json?

[zaggino]

grunt eslint doesn't pass because this pr turns on indentation checks for 4 spaces and there're files outside of this PR not respecting this

[abose]

But the NPM install would also pull the same amount of code right. So if brackets have to be working, the node modules anyways need to be in place.
On second thought, wouldn't this add 10+mb to the installer size?

[zaggino]

Yes, that's true. Installer size would definitely grow.

[ficristo]

I don't see the point of the noop function, can you remove it?
Just leaving the comments // no action required should be enough.

It shouldn't matter much but maybe use regex.test instead of entry.match(/^\.eslintrc(\.(js|yaml|yml|json))?$/); since you aren't using the results.

It is safe this process.chdir(projectRoot); ? I assume it is since running in a node domain.

[zaggino]

process.chdir(projectRoot); has been in the extension for months, never caused any problems ... so I assume it's safe, new version of eslint will support cwd parameter (eslint/eslint@eb1ef88) so this won't be required.

[ficristo]

I don't know if happened with JSLint too, but if I create a new project without a .eslintrc and then I add one I need to restart Brackets to make it take effect.

[zaggino]

@ficristo fixed in 2b57d05

[zaggino]

This died due to lack of feedback.
Latest version currently available here https://github.com/zaggino/brackets-eslint