Use Esprima 2.x.

[ariya]

Facebook's fork is deprecated. Let's just the mainline Esprima 2.x since it already supports major ES6 features.

[ariya]

@thlorenz ping

[thlorenz]

As indicated, I'm weary of merging this in. As far as I recall people were using this branch and things broke horribly (particularly in browserify) so they switched to facebook's fork.

Have those problems been fixed by now?
What would we gain by using this version? Your PR contains no updates to the config which means that no extra keywords would be supported?

[ariya]

Thanks for the response! I'm not sure about the issues from browserify (do you have the link handy?). I can't recall how browserify switched to Esprima 2. It probably attempted to use the so-called harmony branch, which was considered experimental and associated with esprima-fb, or worse, a few others unmaintained forks (e.g. esprima-six). Despite the name, esprima-fb is not a part of official Esprima as it served as a playground for Facebook to add support for ES6 and JSX while the ES2015 specification was not finalized yet (and yes, I wished many times in the past that Facebook didn't call it esprima-fb).

Esprima 2.0 is the beginning of official ES6 support where ES6 features were added bit-by-bit, culminating in the latest 2.7 which has all the syntax features per the latest ES2015 specification. The development happens in Esprima upstream master systematically and carefully, not in some experimental branch. Official releases were made throughout the 2.x series. Other JS tools which need to support ES6 have switched to use it (Istanbul, JSCS, js2coffee, jsfmt, etc). Another way to think about it: Esprima 1.x is an ES5 parser, Esprima 2.x is an ES6 parser. /cc @mikesherov

As for performance, (since a long time ago) Esprima is not much slower nor much faster than anybody else (see e.g. this speed comparison). If we want to compare latest esprima-fb vs Esprima 2.7.0, the numbers follow:

esprima-fb
    Underscore 1.5.2    42.5 KiB      8.49 ms ± 1.66%
      Backbone 1.1.0    58.7 KiB      8.48 ms ± 0.52%
      MooTools 1.4.5   156.7 KiB     53.71 ms ± 4.77%
        jQuery 1.9.1   262.1 KiB     73.78 ms ± 4.66%
          YUI 3.12.0   330.4 KiB     51.56 ms ± 4.29%
 jQuery.Mobile 1.4.2   442.2 KiB    126.34 ms ± 3.38%
       Angular 1.2.5   701.7 KiB    106.40 ms ± 5.12%
                     ------------------------
                      1994.3 KiB    428.75 ms ± 4.33%

Esprima 2.7.0
    Underscore 1.5.2    42.5 KiB      6.91 ms ± 0.83%
      Backbone 1.1.0    58.7 KiB      6.72 ms ± 0.89%
      MooTools 1.4.5   156.7 KiB     31.54 ms ± 0.60%
        jQuery 1.9.1   262.1 KiB     33.61 ms ± 0.68%
          YUI 3.12.0   330.4 KiB     28.35 ms ± 0.58%
 jQuery.Mobile 1.4.2   442.2 KiB    103.00 ms ± 5.55%
       Angular 1.2.5   701.7 KiB     86.60 ms ± 6.57%
                     ------------------------
                      1994.3 KiB    296.73 ms ± 4.84%

Esprima 2.x also aligns its syntax tree closer to the de-facto AST format, ESTree. This is the same AST followed by other parsers. Since esprima-fb is not maintained anymore, its AST is slightly outdated.

Also, Esprima 2.x tries very hard to follow the ES2015 specification faithfully. Take a look at the way it correctly handles e.g. yield, identifier names, SMP, let, etc (with tons and tons of new unit tests). Those, along with many other spec-compliant fixes, were definitely not part of esprima-fb. In the context of redeyed (as well as e.g. cardinal), I think it's quite important to handle thoses cases well.

I'm not aware of the config.js file, I'll take a look. Meanwhile, I'm happy to answer more questions.

[mikesherov]

I can say that JSCS has been using 2.x for a while and are quite happy with the results.

As far as what there is to be gained, rejoining the active mainline development now prepares this codebase for the eventual ES2016 changes that esprima will support but esprima-fb will not, since it's dead.

As far as concerns of regressions go, the concern would be people using redeyed to highlight jsx. Otherwise, I think the unit suite that exists covers it pretty well, but of course, that's your call.

Thanks for considering this PR.

[thlorenz]

tl;dr not seeing a huge benefit, but willing to merge if you feel strongly about it and ensure that cardinal keeps working.

Correct me if I'm wrong, but if I read your comment correctly @ariya we'd just prepare for gains here, but wouldn't see any right away.

• performance wouldn't change
• AST would be more compatible, but redeyed relies mostly just on tokens, the AST just being passed for advanced processing

rejoining the active mainline development now prepares this codebase for the eventual ES2016 changes

OK, but nothing will be practically gained until the config file is updated since otherwise those extra tokens will be ignored.

So with all this considered I'm leaning towards upgrading only when those additions become practical enough to allow adding extra tokens to the config file that then are handled by redeyed.

However if you feel very strongly about this I'm open to merge this anyways especially considering that only 3 packages depend on redeyed currently (the main one being cardinal). So not too much could break here.
Please make sure that the cardinal tests still pass after upgrading redeyed and ideally update the redeyed config and the cardinal themes which are basically redeyed config files.

[ariya]

I've added some future and strict mode reserved words to config.js. I did check cardinal with this modified redeyed and it looks good. I will create a pull request for cardinal themes to match the updated config.js, if this one looks OK to you.

[ariya]

The benefit of using Esprima 2.7 instead of esprima-fb is that redeyed will correctly understand the input (JavaScript program or module) according to the specification.

Perhaps it was not obvious from my previous comment, I'll try to clarify it.

The fact that esprima-fb is outdated (it stopped tracking the specification long time ago) means that it is not as specification-compliant as Esprima 2.7. Among others, it means that some valid ES2015 program and module will be incorrectly rejected by esprima-fb (and thus also redeyed as well as cardinal). Few cases off the top of my head (each line is an example):

function f() { new.target() }
x = 09.5
([[{ x= y }]]) => z;
for (var x = y in z) f(x);
function *x() { (z = yield) => {} }
import {x, y,} from "z";

Whenever I use cardinal and it mistakenly handles the input, it makes me sad (hence why I'm motivated to propose this fix).

If you decide to merge this, I certainly hope it is because you are convinced of the said benefit, not merely because I (or someone else for that matter) feel strongly about it.

[ariya]

Another good example of real-world highlighting problem: https://github.com/facebook/esprima/issues/117.

[thlorenz]

Made you collab @ariya.
So please merge to master and ping me so I can version and publish.
BTW forward merges only please more info here.

[ariya]

Thanks for your vote of confidence, @thlorenz! I'll merge it soon.
And yes, I'm also a big fan of fast-forward merge.

[thlorenz]

Thanks, published as 0.6. Will update cardinal now to use it.

[thlorenz]

Upgraded and updated cardinal themes so the change should've rippled through there :)

[ariya]

Excellent. Thank you @thlorenz! 👍