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

Investigate supporting ES6/JSX #1291

Closed
nzakas opened this issue Sep 23, 2014 · 86 comments
Closed

Investigate supporting ES6/JSX #1291

nzakas opened this issue Sep 23, 2014 · 86 comments

Comments

@nzakas
Copy link
Member

@nzakas nzakas commented Sep 23, 2014

After many requests and some soul-searching, I've come around to believing that ESLint should optionally support JSX. My rationale is based on:

  1. The growing popularity of React/JSX
  2. The existence of Esprima-FB and estraverse-FB

There is still the issue of escope not supporting es6 scoping rules and relying on estraverse directly, but that may be solvable.

The way I'm envisioning this working is by allowing you to specify one of four JavaScript modes:

  1. ECMAScript 3
  2. ECMAScript 5
  3. ECMAScript 6
  4. JSX

That means ECMAScript 6 support is separate from JSX support. You opt-in to JSX, you get whatever is in Esprima-FB vs. opting in to ES6, which should not support JSX syntax.

This issue is to gather investigation data to determine how this can be achieved and what blockers might exist.

@michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Sep 24, 2014

ES4? I hope you meant ES5.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Sep 24, 2014

Yikes, yes.

@michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Sep 24, 2014

ECMAScript 6 is a superset of ECMAScript 5, which is a superset of ECMAScript 3. I see only two options: ES6 and JSX.

@andreypopp
Copy link
Contributor

@andreypopp andreypopp commented Sep 24, 2014

@nzakas thank you for reconsidering this!

Another library which can provide traversal over es5/es6/jsx/es7 is ast-types which is used by recast.

@vjeux
Copy link

@vjeux vjeux commented Oct 1, 2014

Please note that we're committed to JSX and in order to make sure that the integration with tooling such as eslint is as easy as possible we created a specification for JSX.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Oct 1, 2014

@vjeux good to know! The spec is useful, but not as useful as libraries that we can plug in and have "just work". :) Have you (all) done any work around creating an escope that's compatible with JSX?

@jeffmo
Copy link

@jeffmo jeffmo commented Oct 1, 2014

@nzakas: It's worth noting that esprima-fb is the esprima harmony branch + JSX (it tracks the upstream harmony branch, we periodically merge in updates from upstream, and we always send harmony-specific PRs to upstream first) -- so if you just always use esprima-fb for ES6, you're supporting both ES6 and JSX. If you were to provide a toggle between esprima-fb and esprima/harmony, the only distinction you'd see (barring bugs) is that esprima/harmony would give a parse error if you threw JSX at it.

On the other hand, you still have the option of rejecting JSX in code if you so desire -- even if the parser parses it (if you see the presence of a JSX node while traversing the syntax tree, you error).

Would it make sense to simply provide a no-JSX lint than a toggle on which parser to use?

@vjeux
Copy link

@vjeux vjeux commented Oct 1, 2014

The only issue using esprima-fb (or the esprima harmony branch) applied on ES5 JS code is the following:

lib/rules/no-extra-parens.js
  43:12  error  Expected a "break" statement before "case"  no-fallthrough
  53:12  error  Expected a "break" statement before "case"  no-fallthrough

The harmony branch has the fallthrough comment at a different position in the AST. Shouldn't be hard to get the lint rule accept both positions.

In order to lint ES6 or JSX code, there are a few rules that need to be updated. For example the unused variable rule doesn't consider to be a 'use' and marks it as unused.

We're currently playing around switching from jslint after pre-processing to eslint. So far we've got something "working" by disabling a few rules. It doesn't seem that it would require drastic changes and only a handful of pull requests would be needed to support it properly.

@sophiebits
Copy link

@sophiebits sophiebits commented Oct 1, 2014

In order to lint ES6 or JSX code, there are a few rules that need to be updated. For example the unused variable rule doesn't consider to be a 'use' and marks it as unused.

This is tricky because in React's JSX, writing <Button> counts as a use of the variable Button but since JSX itself doesn't specify semantics, that might not be true with a different transformer.

@jeffmo
Copy link

@jeffmo jeffmo commented Oct 1, 2014

The harmony branch has the fallthrough comment at a different position in the AST

This sounds like a bug we should just get fixed -- not quite sure why they differ

For example the unused variable rule doesn't consider to be a 'use' and marks it as unused.

This would be a React-specific lint rule. JSX is just syntax -- React applies meaning to that syntax; So given that the lint-rule would be react-specific anyway, specifying the react-specific contextually-local variables in the lint rule makes sense.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Oct 1, 2014

@jeffmo ah, that's very helpful information to know! I didn't realize the relationship between esprima and esprima-fb was so well-defined. Your point about erroring on JSX syntax is valid, too. It's kind of nice not to have to do anything and have the parser blow up, but in lieu of that, a hand-crafted approach that just looks for JSX and reports an error is definitely feasible.

A followup question: is ES5 + JSX a valid use case for anyone? I was under the (perhaps mistaken) impression that ES6 was a requirement. If that's not the case, then it would be better to have a JSX opt-in in addition to specifying which ES version you want.

@vjeux that's great. Feel free to file issues for anything that would have to be tidied up to work with esprima-fb. (We require issues for all code-based PRs, so best to get started early. :) )

@jeffmo @vjeux also agree that the comment being in a different place seems like a bug, can one of you follow up on that?

@jeffmo
Copy link

@jeffmo jeffmo commented Oct 2, 2014

@nzakas: I don't think there's anything that says React+JSX strictly requires ES6 today, but it's likely to start building on more and more newer ES concepts (classes, spread operators, arrows, etc).

On the other hand (as @michaelficarra mentioned), in the same way that esprima-fb is a superset of esprima/harmony, ES6 is a superset of ES5 -- so I'd imagine you could apply the same regressive thoughts around ES5+JSX just working off of the esprima-fb parser (unless you're just relying on the parser to blow up if it hits ES6). Though even if you did want such a parser (ES5+JSX, but no ES6), we don't have any plans to build or support such a parser on our end.

@jeffmo
Copy link

@jeffmo jeffmo commented Oct 2, 2014

On the comment-node issue: I'll try to take a look tomorrow

@frantic
Copy link

@frantic frantic commented Oct 4, 2014

Regarding the problem with the comment location, looks like eslint uses esprima 1.2.2. On esprima master (2.0.0-dev) the output differs from esprima 1.2.2. I used following source to test

switch(foo) { case 0: a(); /* falls through */ case 1: b(); }

image

esprima-fb has the same output as esprima 2.0.0-dev

@nzakas
Copy link
Member Author

@nzakas nzakas commented Oct 4, 2014

@frantic yup, that's exactly the problem. There's no reason for the comment location to differ between the two branches.

@nzakas nzakas changed the title Investigate supporting JSX Investigate supporting ES6/JSX Oct 5, 2014
@nzakas
Copy link
Member Author

@nzakas nzakas commented Oct 5, 2014

I took a look at dropping-in esprima-fb and estraverse-fb and basically found two issues:

  1. The location of comments isn't just different for the case statement mentioned previously, it's also different in the treating of top-level comments. This basically looks how estraverse.attachComments() deals with comments rather than the way esprima 1.2.x deals with comments, which makes many rules dealing with comments break in those cases.
  2. esprima-fb is a bit slower than esprima 1.2.x. I'm not sure if that's because the enhancements from 1.2.x didn't land in the Harmony branch, but it's a non-trivial performance reduction. You can try yourself running npm run perf.

Here's with esprima 1.2.2:

CPU Speed is 3093 with multiplier 7500000
Performance Run #1:  1719.9273899999998ms
Performance Run #2:  1716.118489ms
Performance Run #3:  1708.291122ms
Performance Run #4:  1688.523239ms
Performance Run #5:  1706.549722ms
Performance budget ok:  1708.291122ms (limit: 2424.8302618816683ms)

Here's with esprima-fb:

CPU Speed is 3093 with multiplier 7500000
Performance Run #1:  1893.5033819999999ms
Performance Run #2:  1884.1219959999999ms
Performance Run #3:  1904.216641ms
Performance Run #4:  1897.484768ms
Performance Run #5:  1899.810498ms
Performance budget ok:  1897.484768ms (limit: 2424.8302618816683ms)
nzakas added a commit that referenced this issue Oct 6, 2014
@nzakas nzakas modified the milestones: v0.10.0, v0.9.0 Oct 6, 2014
@nzakas
Copy link
Member Author

@nzakas nzakas commented Oct 6, 2014

I started some investigative work around this on the esprimafb branch. Not anywhere near complete, but gives a decent idea of what I'm thinking.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Oct 26, 2014

Filed a ticket with Esprima about the comment attachment problem: https://code.google.com/p/esprima/issues/detail?id=607

@nzakas nzakas added this to the v0.10.0 milestone Oct 26, 2014
@MoOx
Copy link
Contributor

@MoOx MoOx commented Dec 5, 2014

Any update on this? I'm seeing commit under the yannickcr/eslint#deezer fork, but can't make it works (I've tried the "valid-syntax" option with jsx to true :/)

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 5, 2014

What kind of an update are you looking for?

There's no valid-syntax rule. See http://eslint.org/blog/2014/11/es6-jsx-support/ for how to configure correctly and what's going on.

@MoOx
Copy link
Contributor

@MoOx MoOx commented Dec 5, 2014

I just found a way (not sure why) to force jsx

{
  "settings": { "jsx": true }, // had to add this
  "valid-syntax": [2, { "jsx": true }],
}
@MoOx
Copy link
Contributor

@MoOx MoOx commented Dec 5, 2014

Well not sure where I found that haha. Sorry for this
The other part was to do that

$ eslint . --ext=.js --ext=.jsx

But thanks now it's working correctly! Awesome work.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 16, 2014

Update: after much deliberation, I've decided the best way to move forward is to fork Esprima and make the changes we need. We have been held back by Esprima bugs for too long. So here is our brand new adding Esprima fork called Espree: https://github.com/eslint/espree

More details on the readme, but basically I'm starting with esprima 1.2.2 and adding in ES6 and JSX features. 1.2.2 is the version that ESLint runs on today, so it provides the best compatibility for us going forward.

Unlike Esprima, Espree will support JSX as an option directly. @vjeux if Facebook folks are willing to help add JSX support to Espree, I'll happily accept the contribution.

For ES6, it's just a matter of pulling in features one by one from the Esprima harmony branch. I've already added a way to specify the ECMAScript version to use, restricted let and const to ES6 more, and added support for the regular expression u and y flags. Overall progress is tracked here: eslint/espree#10

It's disappointing to need to create a new parser, but JSHint and JSLint already have their own parsers, so this brings us inline and allows us to move much faster.

@lencioni
Copy link
Contributor

@lencioni lencioni commented Dec 16, 2014

Could you use Facebook's fork of esprima, which already has full support for JSX, instead of forking? I don't have a lot of context here, so forgive my ignorance.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 16, 2014

That's what we've been trying to do, but Esprima-FB is based on the Esprima harmony branch, which is where all the problems lie. It's just not worth continuing down that path.

@lennartcl
Copy link

@lennartcl lennartcl commented Dec 16, 2014

@nzakas Are you sure you're not going with Acorn? Acorn would give you full es6 support, better parsing speed (especially with location info enabled), and error recovery. I'm currently considering changing from jshint to eslint on c9.io, but particularly that lack of error recovery is pretty obvious when all warnings disappear whenever I'm typing and there's a parse error :( (Also, if you would switch to Acorn I would get the benefit of using just one parser for all javascript analysis rather than having two of them ^_^;; But then I might run into #1025, I guess.)

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 16, 2014

I'd love to use acorn, but it has several incompatibilities with esprima that would require changes to almost every rule we have (not to mention it would break custom rules that people have written).

@andrewdeandrade
Copy link
Contributor

@andrewdeandrade andrewdeandrade commented Dec 16, 2014

An alternative is to punt on jsx entirely and use a library like https://github.com/alexmingoia/jsx-transform/ to convert the jsx to hyperscript and then run that file through the linter. The one complexity that remains is making sure that the jsx conversion produces hyperscript code that follows the same rules as your eslint configuration.

@lennartcl The esprima API does allow you to pass in an optional tolerant flag on parse that when true makes parse return "an extra array containing all errors found, attempts to continue parsing when an error is encountered". See http://esprima.org/doc/index.html

Is the esprima tolerant mode sufficient for your needs in c9.io?

@michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Dec 16, 2014

@nzakas if you've forked esprima and made bugfixes, why not send a PR back upstream as the esprima-fb developers do, instead of renaming your esprima fork to espree?

@xjamundx
Copy link
Contributor

@xjamundx xjamundx commented Dec 16, 2014

@michaelficarra I'm pretty sure the answer (as is stated in the thread) is that the Harmony branch is pretty incompatible with main branch used by ESLint and it's not fair to ESLint rule makers out there to change the AST at this point.

See also: eslint/espree#12

@michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Dec 17, 2014

I'd like to see a list of those incompatibilities. They're probably things that esprima would care to fix.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 17, 2014

@michaelficarra I've forked off of 1.2.2, which is pretty different than 2.0.0, so it's not going to be easy to reconcile the changes. I've also been filing bugs on the Esprima bug tracker and getting no updates and no indication if a fix is forthcoming. It's not fair to keep holding ESLint back waiting for changes that may or may not happen.

In any event, and to be clear to everyone: this decision has been made. I've been trying to work with Esprima for a while now and have reached the point where I don't see using Esprima as a benefit any longer. ESLint needs to move forward and the best way to do that is to control the parser we're using. Hence Espree.

@andrewdeandrade
Copy link
Contributor

@andrewdeandrade andrewdeandrade commented Dec 17, 2014

@nzakas are these outstanding issues eslint has with esprima all related to getting JSX support in eslint or are there unresolved issues with esprima with vanilla javascript linting as well?

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 17, 2014

@malandrew the Esprima issues have nothing to do with JSX support. The fact that esprima-fb is based on the Esprima harmony branch means any issues from Esprima leak through. That's the problem (more details throughout this thread).

@lennartcl
Copy link

@lennartcl lennartcl commented Dec 17, 2014

@malandrew I experimented with the "tolerant" option. But for most things I throw at it, it just fails with an exception anyway. It might be nice for eslint to at least try enable that option by default though, if only to show multiple parse errors at the same time.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Dec 17, 2014

If you're interested in discussing the tolerant option, please open a separate issue. That topic is orthogonal to this one.

@nzakas
Copy link
Member Author

@nzakas nzakas commented Jan 10, 2015

#1602 adds the ability to set the feature flags for Espree, which allows turning on ES6 features and JSX.

#1640 makes sure traversal of JSX nodes works correctly

@nzakas
Copy link
Member Author

@nzakas nzakas commented Jan 18, 2015

Closing as 0.12.0 introduced support for JSX and partial ES6 support.

@nzakas nzakas closed this Jan 18, 2015
@StoneCypher
Copy link

@StoneCypher StoneCypher commented Jan 18, 2015

🏆

@eslint eslint locked and limited conversation to collaborators Jan 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.