Use Espree instead of esprima to support newer ES specs

[Markus-ipse]

All tests still pass and I verified that it works by npm linking this branch to a local version of cardinal, which I then linked to a project where I successfully highlighted code that use the stage 2 proposal for object spread syntax.

Fixes thlorenz/cardinal#16

[ariya]

Oh, why do we want to go back to the unmaintained fork of Esprima again 😢 Please read PR #4 for more details.

[Markus-ipse]

Didn't know Esprima-fb had been used before in this repo. It was the easiest drop-replacement for Esprima that I could find that supports object-spread operator and JSX syntax.

@ariya Is it possible to get Esprima to support stage 2+ ES features and JSX syntax, or do you know of a Esprima-compatible parser that does?

[ariya]

Projects following ESTree (e.g. Espree) might work for you.

OTOH, the best way to support various syntax is to have a fallback/tolerant mode: if the input can't be parsed, try to tokenize the input.

[Markus-ipse]

I'll give Espree a shot then :)

[Markus-ipse]

Ok, I've rewritten the PR to use Espree instead of Esprima-fb now. Should I replace all references to 'esprima' with 'espree' or should I leave it as is?

[Markus-ipse]

Hmm.. Some tests seem to fail in travis-ci, but all tests pass locally..

Locally there seems to be 35 tests that runs (and passes) in redeyed-smoke.js

whereas in travis there's a lot more (1829) tests being run in redeyed-smoke.js

Anyone has any ideas as to why?

Edit: Found it! It was due to npm v2.x being used in travis, which doesn't try to flatten the node_modules folder. Therefore all espree dependencies where also tested

[ariya]

I can't comment much on this due to conflict of interest (I'm the author of Esprima).

Notwithstanding that, I still think the right fix is to fallback to pure tokenization mode. Otherwise, we'll keep switching parser again in the future.

[Markus-ipse]

@ariya I'm not even sure what that means, is that something that should be fixed in esprima or in redyed? Could you point me in the general direction of where I should start looking to implement the above mentioned fallback?

[ariya]

@hummlas Let me write it up as a new issue.

[thlorenz]

Yeah someone else would have to write that tokenizer, I don't have bandwidth for it. Or is pure tokenization mode a feature of esprima feature I'm not aware of @ariya?

IMO if all tests pass + it now supports ES6 features that's a gain, so I'm for that.

However I'm not happy with the last commit @hummlas, just running less smoke tests is not a good idea. Instead adapt the .travisci.yml script to include

before_install:
  - npm install -g npm@2

see here.

That way we know it's always using npm2. The tests should pass with unflattened folders (they did before). If something is failing that means there is a regression and some files that could be parsed before can't be parsed anymore. You should check if they also fail when using the parser we used before.

Also please update this config to include the added ES6 tokens. We'll have to do this similarly for cardinal themes before it takes effect there. Once that's done I'm willing to merge this for now even though that makes @ariya sad.

And thanks for you patience! .. hang in there, it'll all come together ;)

[ariya]

Or is pure tokenization mode a feature of esprima feature I'm not aware of @ariya?

Yes! It already exists, Esprima can tokenize an entire input stream without parsing it. This is what I plan to write in more details, because I think this is the proper long-term solution for redeyed.

[Markus-ipse]

@thlorenz There were a few files (in the nested node_modules folder) that cause false positives for the occurrence var declarations, for instance a comment containing the name 'Ingvar`.

I tried to just exclude the files that caused the false positives for var occurrences, and that solved the problem, at least locally.. When it runs in travis the test times out, probably because there are so many (1791) files to traverse in the nested node_modules folder (see https://travis-ci.org/thlorenz/redeyed/jobs/152034277).

All that being said it's probably better to go with @ariya's solution in #8 :)

[thlorenz]

@hummlas well that's a bug in the parser and thus those tests demonstrate that things break currently if we use it. We can't start using that parser unless that's fixed, otherwise we'll get these false positives when people use it as well.

I tried to just exclude the files that caused the false positives

Not a good idea to do that, these files show a bug in the parser. That's exactly why I added the smoke tests to detect those edge cases.

Bottom line is things need to work after the parser change without changing any tests.

[Markus-ipse]

@thlorenz I don't think it's a bug in the parser. I think it's due to the second part of the assert in the test-case:

t.assert(~result.indexOf('+var-') || !(~result.indexOf('var ')

in redeyed-smoke.js

The below part should make sure that the 'var' part of all var-statements are replaced with '+var-':

result = redeyed(code, { Keyword: { 'var': '+:-' } }).code

e.g. var foo; becomes +var- foo;

But the assert checks if the result contains a successfully transformed var ('+var-') OR if there were no var to transform to begin with, in which case the test is also counted as passing.

The problem is however that !(~result.indexOf('var ') will tell us that there were indeed a 'var' that should have been transformed, if our code contains something like:

 // Created by Ingvar Kamprad

even though the 'var ' that was found wasn't part of var-statement, and thus shouldn't have been transformed.

Please correct me if I'm wrong though :)

[ariya]

This is now overtaken by event, see PR #9.