c9 / core

Cloud9 Core - Part of the Cloud9 SDK for Plugin Development https://c9.github.io/core/ https://c9.io
Other
2.56k stars 922 forks source link

Update ESLint #251

Open jeffshaver opened 8 years ago

jeffshaver commented 8 years ago

From the looks of it, you are still running a forked version of 0.10.x... They are one 2.x already and this is causing some major pain for my projects on c9. Almost enough to push my teams away from c9.

nightwing commented 8 years ago

We use 2.x https://github.com/c9/c9.ide.language.javascript.eslint/blob/master/worker/eslint_browserified.js Could you describe in more detail what issues do you see?

jeffshaver commented 8 years ago

@nightwing sorry about that. I thought i saw somewhere that it was a branch of 0.10 (https://github.com/c9/core/blob/master/package.json#L22). My issue is that basically in the c9 editor, eslint isn't understanding that we have plugins in our .eslintrc file. And it is also not using the correct parser (from what I can tell). I am using babel-eslint as the parser, which shouldn't say that the decorator syntax is invalid:

@Radium
class MyComponent extends Component {}

And I get a ton of errors at the top saying it couldn't find the rules for every rule i specify in my eslintrc file that use the eslint-plugin-react plugin.

Is this the wrong place for this? I haven't gotten any feedback on community.c9.io so I started looking for other places to get answers:

I thought that maybe it was the version of eslint that was causing issues with plugins... Any ideas about how to fix it or where to go to find out?

Decorator error:

screen shot 2016-02-18 at 3 14 44 pm

react plugin errors:

screen shot 2016-02-18 at 3 15 48 pm
Pike commented 8 years ago

I wondered about this, too. I didn't find the use case we have for eslint in package.json, tbh. Is that actually still used? I think for the js mode, we use the in-browser copy, and I have no idea how one would tell that one about plugins, I'm kinda suspecting one doesn't? And then, there's the thing about eslint only learning about plugins per install of eslint, aka eslint/eslint#1238

nightwing commented 8 years ago

Indeed, eslint is package.json is redundant, we can remove it.

we use in browser copy of eslint and react-eslint, however due to error it expects rule names to be "jsx-uses-react" instead of "react/jsx-uses-react"

@jeffshaver sorry we have missed your question on community@c9.io

Currently we do not have a way to support babel-eslint parser and we do not fully support config.extends and files in subdirectories.

We can add support for disabling in browser eslint and running it from vm but that will be slower, somewhat similar to the way python/php/go linting works now.

Pike commented 8 years ago

re in the in-tree eslint, I think that's actually useful for linting c9 code. eslint 1.10.3 at least breaks on the .eslint rules. https://github.com/c9/core/commit/f42987744b9b47eeb90a86cb3782bba9e6428c4d has added a plugin dependency which isn't matched yet, though.

I wonder if there are heuristics about the eslint config that one can use to decide which version of eslint to use, and if it wants to be remote or in-client?

jeffshaver commented 8 years ago

@nightwing How slow would moving it to the vm be? It seems like that would be the only viable option for my team. The current setup is basically doing nothing since it thinks there are parsing errors in almost every file :/

nightwing commented 8 years ago

@jeffshaver we found a way to support decorators and react plugin in default configuration (the fix will be online in a day or two). Are there other syntax features/ plugins you use?

@Pike we do not use eslint from commandline, we use it only from ide. That's why the version in package.json is outdated and plugins are missing.

It was added to package.json because when we were experimenting with browserifying it on demand, but that didn't work well.

I wonder if there are heuristics about the eslint config that one can use to decide which version of eslint to use, and if it wants to be remote or in-client?

we could check for plugins we do not support.

jeffshaver commented 8 years ago

@nightwing i use several es7 features. I can't test them since I can't get past the react plugin thing. one that i use a lot is object spread. I also use the new bind operator... ::this.handleChange. But there are a bunch I use frequently.

one question... will i need to remove the react/ before the rules in order to get it to stop erroring? If I have to do something other than the default eslint config, that prevents it from working everywhere else, which causes issues and wouldn't be worth it.

nightwing commented 8 years ago

spread is supported by default parser too.

Looking at http://babeljs.io/docs/plugins/#Experimental, i think we do not support only a few of experimental transforms: do-expressions and function-bind.

will i need to remove the react/ before the rules in order to get it to stop erroring

no, that's a bug in cloud9, which will be fixed. I'll let you know once the pull request is merged and deployed.

jeffshaver commented 8 years ago

Awesome! Thanks. I'll let you know if I run into anymore issues after he fix is deployed On Tue, Feb 23, 2016 at 4:26 PM Harutyun Amirjanyan < notifications@github.com> wrote:

spread is supported by default parser too.

Looking at http://babeljs.io/docs/plugins/#Experimental, i think we do not support only a few of experimental transforms: do-expressions and function-bind.

will i need to remove the react/ before the rules in order to get it to stop erroring

no, that's a bug in cloud9, which will be fixed. I'll let you know once the pull request is merged and deployed.

— Reply to this email directly or view it on GitHub https://github.com/c9/core/issues/251#issuecomment-187918528.

nightwing commented 8 years ago

this was deployed today

jeffshaver commented 8 years ago

@nightwing It does indeed seem to be fixed! I am now having an issue with static class properties: I get this: Parsing Error: Unexpected Token =

If I delete that though, it looks like its working for some files. I am unsure what other ES7 features could cause breakage. Is there a list of ES7 features you don't support?

nightwing commented 8 years ago

Thanks for checking. I do not know the exact list of differences between features supported by babel and acorn. I'll try to add support for loading babel-eslint instead of adding experimental syntax support to acorn.

jeffshaver commented 8 years ago

@nightwing look forward to the update. thanks for looking into all this!

jeffshaver commented 8 years ago

@nightwing any update on the progress of this?

nightwing commented 8 years ago

@jeffshaver using babel-eslint directly didn't work since it does some hacky monkey-patching that doesn't work with browserify. Also babel 6.x have dropped support for decorators So i've continued improving acorn based version instead and now static class properties and do expressions should work. Let me know if you see other issues.

jeffshaver commented 8 years ago

@nightwing we use the :: bind operator and now that is throwing an error. Also, babel 6.x has a plugin to support decorators. I can't remember the specific name.

Thanks for all your help on this

jeffshaver commented 8 years ago

@nightwing also, getting an eslint error about props not being validated but they are validated. specifically:

'children' is missing in props validation. However i have a static class property that validates that prop.

class App extends Component {
  static propTypes = {
    children: PropTypes.node
  }
  // ...
}```
nightwing commented 8 years ago

this two are fixed now.

jeffshaver commented 8 years ago

@nightwing I am still getting a parse error regarding the :: bind syntax.

nightwing commented 8 years ago

Could you show example code? I don't see error with foo = x::y()::z()

jeffshaver commented 8 years ago

@nightwing Sure. Here is some copy/pasted from my app.

<SourceSelect
  style={style.verticalTop}
  onChange={::this.onChangeSource}
/>

Based on the code you provided, if you do this::this.onChangeSource it isn't an error. However, ::this.onChangeSource is valid syntax for this. Apologies for not providing source code for that one when I posted it.

nightwing commented 8 years ago

It parses leading :: s now

jeffshaver commented 8 years ago

@nightwing Indeed that does work. Now I am having issues with object spread (another es7 feature). example:

const x = {
  a: 1,
  b: 2
}

const y = {
  c: 3
}

// this is the spread
const z = {
  ...x, // <-- Throws parse error
  ...y,
  b: 4
}

// output: {a: 1, b: 4, c: 3}
jeffshaver commented 8 years ago

@nightwing also, I updated my version of eslint-plugin-react and it is saying that a rule that was added (react/sort-prop-types, changed from react/jsx-sort-prop-types) isn't a valid rule.

nightwing commented 8 years ago

... was not working because it required experimentalObjectRestSpread in eslint. I've changed it to be the default. I've also added an alias for react/sort-prop-types but generally eslint and plugins seem to be updated too often, i'll probably have to add support for commandline version of the linter.

jeffshaver commented 8 years ago

@nightwing any change on this? I updated eslint on our project (and turned on some of the new rules in the new versions, but they aren't supported, so I am getting a bunch of errors again).

namirsab commented 7 years ago

This issue is still a problem for me and my team. We are using the airbnb styleguide and cloud9 fails when we use (for instance) trailing commas in function calls. It's annoying because after that any other hints or IntelliSense are broken.

Are there any plans on this?

Curclamas commented 7 years ago

@nightwing @namirsab I second this! More and more projects are using Airbnb rules. Using c9 in conjunction with this sadly yields the annoying problems earlier mentioned above.

micahnz commented 6 years ago

Yup pretty much the only thing holding me back right now is that I can't use eslint properly like I am in Atom with Airbnb rules.

micahnz commented 6 years ago

So I really wanted to start using Cloud9 in place of Atom asap so I came up with a fairly quick solution to the linting woes and created a plugin to replace the current plugin.

This plug-in uses eslint_d to eliminate the node.js startup time along with the netcat abilities to lint the code in under ~100ms on the Cloud9 server using the standard eslint cli for full feature support.

It was a rather quick and basic implementation based on my very minimal knowledge of the Cloud9 SDK acquired while building the plug-in but it generally works quite well.

https://github.com/michaelmitchell/c9.ide.language.javascript.eslintd