Skip to content
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

eslint rules needed for cssx #10

Closed
rosskevin opened this issue Jun 7, 2016 · 18 comments
Closed

eslint rules needed for cssx #10

rosskevin opened this issue Jun 7, 2016 · 18 comments

Comments

@rosskevin
Copy link
Contributor

rosskevin commented Jun 7, 2016

I'm using standard and standard-react settings for eslint currently, and eslint is complaining about color: #333.

Sample repo

Are there a specific recommended set of rules we need to configure to be CSSX friendly? I tried looking at your file but it doesn't extend a preset, it is manual.

This is my starting point:

{
  "parser"  : "babel-eslint",
  "extends" : [
    "standard",
    "standard-react"
  ],
  "env"     : {
    "browser" : true
  },
  "globals" : {
    "__DEV__"      : false,
    "__PROD__"     : false,
    "__DEBUG__"    : false,
    "__COVERAGE__" : false,
    "__BASENAME__" : false
  },
  "rules": {
  }
}
@rosskevin rosskevin changed the title eslint integration eslint rules needed for cssx Jun 7, 2016
@rosskevin rosskevin changed the title eslint rules needed for cssx eslint rules needed for cssx and standardjs preset Jun 7, 2016
@krasimir
Copy link
Owner

krasimir commented Jun 8, 2016

Hey @rosskevin,

I'm not sure how to approach this problem. For me it looks like the cssx block should be totally ignored by eslint. Mainly because CSSX is a CSS specific thing while eslint is a tool for JavaScript and JSX. If we want to lint the CSSX we should be probably using csslint (or stylelint) somehow.

I'll leave this open and will try finding some sensible solution.

@rosskevin
Copy link
Contributor Author

rosskevin commented Jun 8, 2016

I think you took this one step further, in that it would be nice to csslint it (which would be nice to have as well!).

My problem is that it is not currently passing eslint (and I have jsx rules configured):

app_js_-af_core-___projects_af_core

@krasimir
Copy link
Owner

krasimir commented Jun 8, 2016

I see. This will be tough. Is there any way to ignore a code block (not just a line).

@rosskevin
Copy link
Contributor Author

That's what I was wondering, but there were so many rules in your own .eslintrc that I couldn't review each one. I wasn't sure if you already knew a specific rule. Would your eslint code for this repo allow the code above to pass?

@rosskevin
Copy link
Contributor Author

rosskevin commented Jun 8, 2016

I know we can disable/re-enable for each, but that seems only a temporary workaround (painful and messy):

/* eslint-disable */
const styles = ...
/*eslint-enable */

EDIT: doesn't work - see below.

@rosskevin
Copy link
Contributor Author

rosskevin commented Jun 8, 2016

This might be tougher than I thought, I added the eslint disable and it still fails:

https://github.com/rosskevin/my-relay-starter-kit/commit/dfa05b2c3c02d2a7698d4108dfd81a42d70a02b2

/Users/kross/projects/my-relay-starter-kit/src/App.js
  20:16  error  Parsing error: Unexpected token

related:

@rosskevin rosskevin changed the title eslint rules needed for cssx and standardjs preset eslint rules needed for cssx Jun 8, 2016
@rosskevin
Copy link
Contributor Author

I agree this looks tough.

I think figuring it out needs to be a priority if CSSX is to be adopted. As-is, failing eslint (parsing) means that a project would either need to toss out eslint validation, or toss out CSSX.

This looks like a huge barrier for adoption.

@rosskevin
Copy link
Contributor Author

A lead: babel-eslint. I'm currently using this already in my starter kit (because I'm using relay):

eslint/eslint#4636

@rosskevin
Copy link
Contributor Author

rosskevin commented Jun 8, 2016

FYI - I'm using cssx-loader with webpack, perhaps I need to try a different process.

@krasimir
Copy link
Owner

krasimir commented Jun 8, 2016

Definitely I'll make eslint work with CSSX. At least not break the other linting.

@rosskevin
Copy link
Contributor Author

rosskevin commented Jun 8, 2016

Seems like the babel-eslint (transpile first then eslint) is the most viable route I am seeing. Is there already a babel plugin for cssx?

@krasimir
Copy link
Owner

krasimir commented Jun 8, 2016

Not really.

@rosskevin
Copy link
Contributor Author

hzoo provided some good background on the #linting slack channel:

hzoo
[9:25] if its not supported by babel then no
[9:26] cssx forks babylon so you would have to basically make your own babel-eslint fork
[9:28] babel/babel#3376 the original pr for cssx was in babel but we decided not to accept it
[9:28] so babel-eslint is going to say unexpected token because the parser doesn’t understand the syntax
[9:29] so you have to plug in your own parser into eslint
[9:29] which is what babel-eslint does - you would just have to do the same with cssx

@krasimir
Copy link
Owner

krasimir commented Jun 8, 2016

Yep, that's what I thought. I'll try finding some time this weekend.

@rosskevin
Copy link
Contributor Author

I read the PR, I agree with all the reasons you have built CSSX. These aren't easy things to tackle so I just want to say thank you upfront for all the effort thus far and looking into this. With the big changes happening in how we create/style front-ends, I think cssx could be a substantial part of that future.

@krasimir
Copy link
Owner

krasimir commented Jun 8, 2016

Oh thank you @rosskevin. It was really fun working on it. The whole parsing and compiling bit was really interesting. I'll try make eslint work with JSX and CSSX in one file. If possible will bring some css linting to the table.

@krasimir
Copy link
Owner

@rosskevin The problem should be fixed now. Please use eslint-plugin-cssx. It's a plugin for ESLint that transforms the CSSX expressions to empty objects. For example:

var styles = <style>
  body {
    margin: 10px;
  }
</style>;

to

var styles = {



};

It keeps the number of lines used by CSSX so the rest of the errors/warnings (if any) are reported at the right line.

@krasimir
Copy link
Owner

I didn't add CSSLint even though this is possible (and quite easy) but CSSX is not example a valid CSS. So this integration may cause more issues then it solves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants