[CLOSED] add ESLint as a default Brackets extension

[core-ai-bot]

Issue by zaggino
Wednesday Dec 09, 2015 at 03:54 GMT
Originally opened as adobe/brackets#11988

---

ref: adobe/brackets#11984

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

---

zaggino included the following code: https://github.com/adobe/brackets/pull/11988/commits

[core-ai-bot]

Comment by zaggino
Wednesday Dec 09, 2015 at 05:00 GMT

---

@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.

[core-ai-bot]

Comment by petetnt
Wednesday Dec 09, 2015 at 14:00 GMT

---

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)

[core-ai-bot]

Comment by zaggino
Wednesday Dec 09, 2015 at 23:48 GMT

---

@petetnt adobe/brackets@634160c should fix that issue.

[core-ai-bot]

Comment by petetnt
Thursday Dec 10, 2015 at 06:55 GMT

---

adobe/brackets@634160c fixed it, thanks!

[core-ai-bot]

Comment by ficristo
Thursday Dec 10, 2015 at 08:59 GMT

---

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.

[core-ai-bot]

Comment by zaggino
Monday Dec 14, 2015 at 23:08 GMT

---

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

@ficristo fixed indentation to 4 spaces in adobe/brackets@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

[core-ai-bot]

Comment by zaggino
Monday Dec 14, 2015 at 23:16 GMT

---

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

[core-ai-bot]

Comment by zaggino
Monday Dec 14, 2015 at 23:27 GMT

---

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

[core-ai-bot]

Comment by zaggino
Tuesday Dec 15, 2015 at 04:20 GMT

---

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

[core-ai-bot]

Comment by ficristo
Tuesday Dec 15, 2015 at 09:26 GMT

---

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?

[core-ai-bot]

Comment by zaggino
Tuesday Dec 15, 2015 at 09:49 GMT

---

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

[core-ai-bot]

Comment by abose
Tuesday Dec 15, 2015 at 10:55 GMT

---

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?

[core-ai-bot]

Comment by zaggino
Tuesday Dec 15, 2015 at 11:47 GMT

---

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

[core-ai-bot]

Comment by ficristo
Tuesday Dec 15, 2015 at 13:25 GMT

---

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.

[core-ai-bot]

Comment by zaggino
Tuesday Dec 15, 2015 at 20:27 GMT

---

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.