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

JSX support #47

Closed
RichardLitt opened this issue Feb 27, 2015 · 21 comments

Comments

@RichardLitt
Copy link
Contributor

commented Feb 27, 2015

So, I'm trying to use standard on .jsx files. And I run into an issue where I can't use //jscs:disable and /* eslint-disable */, because they still depend on the AST parser and will throw an error if they can't parse the JSX templating.

Example file:

, render: function() {
    //jscs:disable
    /*eslint-disable */
    return <div className="form-horizontal">
      {this.renderTextInput('subject', 'Subject')}
      {this.props.email && this.renderTextInput('email', 'Email')}
      {this.renderTextInput('message', 'message')}
    </div>
  //jscs:enable
  /*eslint-enable */
  }

What do you think about this? I could potentially use fs.readFileSync(__dirname, '/form-template.jsx') and pipe the rendering jsx template through that, but that seems messy. Do you think there would be a way of using standard on these files, otherwise?

@feross feross changed the title Parse errors JSX support Feb 27, 2015

@RichardLitt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2015

There ought to be a way to add esprima linting to jscs: cf jscs-dev/node-jscs#708.

I tried adding ecmaScript: {..., "jsx": true} to the .eslintrc - didn't seem to work out of the box. More details on eslint and JSX here.

@gaearon

This comment has been minimized.

Copy link

commented Mar 2, 2015

Disabling linting for JSX blocks is a mistaken approach. You won't catch potential issues in them, and constants you use there will appear unused to the linter. The right approach is to use JSX-aware linter, or pipe compiled JSX to linter.

@RichardLitt

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2015

@gaearon agreed. Do you have any experience making JSX linters work here, like I looked into above? Help would be great.

@gaearon

This comment has been minimized.

Copy link

commented Mar 2, 2015

Wrote an article on teaching ESLint to parse ES6/JSX/some ES7 with Babel a few days ago: https://medium.com/@dan_abramov/lint-like-it-s-2015-6987d44c5b48

It won't help you with standard though..

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

Validated using "jsx":true in the "ecmaFeatures" section of .eslintrc works great, however its not as clear with jscs. I couldn't find any explicit options.

In searching around their issues, it looks like some folks are suggessing using jsxcs as an alternative. running jsxcs with the current rc file does not show a promising result:

Error: Unsupported rules: disallowOperatorBeforeLineBreak, requireSpacesInForStatement

So... once #45 is done and only eslint remains, jsx support should be possible.

@feross feross referenced this issue Mar 4, 2015
3 of 4 tasks complete

feross added a commit that referenced this issue Mar 7, 2015

Add jsx support
For #47.

Not complete until jscs (which doesn't support jsx) is removed.

feross added a commit that referenced this issue Mar 9, 2015

Add jsx support
For #47.

Not complete until jscs (which doesn't support jsx) is removed.

feross added a commit that referenced this issue Mar 12, 2015

Add jsx support
For #47.

Not complete until jscs (which doesn't support jsx) is removed.
@HenrikJoreteg

This comment has been minimized.

Copy link

commented Mar 17, 2015

+1 if this gets resolved I can tell folks to use this in my upcoming workshop at FluentConf.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

I'll take another look at adding support for JSX.

Can someone point me to a couple repos with JSX in them? If they already use standard style, even better!

@RichardLitt

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2015

@feross Here's mine: https://github.com/BeagleLab/beagle/tree/master/js

Already standardized.

@HenrikJoreteg

This comment has been minimized.

Copy link

commented Mar 17, 2015

@feross also adding you to the little demo app I started on for fluent.

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

Looks like jscs should support jsx now, but its not yet been pushed to npm.
jscs-dev/node-jscs#1132

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

OK, so this works, however it depends on jscs in github. The trick is using esprima-fb

Flet@5fb221e

Detailed by jscs contributor here:
jscs-dev/node-jscs#1131

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

https://github.com/BeagleLab/beagle/blob/master/js/views/graphModal.jsx

old n busted:

Error: Use JavaScript Standard Style (https://github.com/feross/standard)
../beagle/js/views/graphModal.jsx:2:4: Modal is defined but never used
../beagle/js/views/graphModal.jsx:3:4: PublicationList is defined but never used
../beagle/js/views/graphModal.jsx:7:18: Missing space before function parentheses.
../beagle/js/views/graphModal.jsx:9:7: Unexpected token <
../beagle/js/views/graphModal.jsx:26:0: Multiple blank lines not allowed.

new hotness:

node bin/cmd.js ../beagle/js/views/graphModal.jsx
Error: Use JavaScript Standard Style (https://github.com/feross/standard)
../beagle/js/views/graphModal.jsx:2:4: Modal is defined but never used
../beagle/js/views/graphModal.jsx:3:4: PublicationList is defined but never used
../beagle/js/views/graphModal.jsx:7:18: Missing space before function parentheses.
../beagle/js/views/graphModal.jsx:26:0: Multiple blank lines not allowed.

feross added a commit that referenced this issue Mar 17, 2015

Fully support JSX
This commit makes jscs correctly parse JSX without errors. (issue #47).

eslint already parses JSX correctly.
@feross

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

Just published 3.1.0 with JSX and React support.

In order to get things like proper detection of used variables inside of JSX, I added eslint-plugin-react. Like with standard@1.0.0, let's start with all React rules enabled and relax them as necessary. Open a new issue if you think a rule is too aggressive.

@HenrikJoreteg, note: I noticed an eslint bug that crashes standard while trying to check your demo app. Here's the eslint issue: eslint/eslint#2060 This isn't related to JSX support :)

I think this issue can be closed now! 👍

@feross feross closed this Mar 17, 2015

@feross

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

@Flet thanks for looking into this - looks like we were working in parallel :)

@HenrikJoreteg

This comment has been minimized.

Copy link

commented Mar 17, 2015

woot! thanks @feross @Flet you're awesome.

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

Excellent! Good call on the eslint-plugin-react include :)

@feross

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

Just released 3.1.1 to automatically lint files that match **/*.jsx in addition to the default of **/*.js. #68

@feross

This comment has been minimized.

Copy link
Member

commented Mar 18, 2015

@HenrikJoreteg The eslint issue I mentioned (eslint/eslint#2060) is fixed and released now :)

@HenrikJoreteg

This comment has been minimized.

Copy link

commented Mar 18, 2015

just noticed that, thanks @feross :)

@RichardLitt

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2015

Works for me! Thanks all!

@anthonybrown

This comment has been minimized.

Copy link

commented Aug 21, 2016

Works great for my React projects!

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
6 participants
You can’t perform that action at this time.