benjamn / recast

JavaScript syntax tree transformer, nondestructive pretty-printer, and automatic source map generator
MIT License
4.93k stars 346 forks source link

esprima-fb → esprima? #178

Closed mathiasbynens closed 8 years ago

mathiasbynens commented 9 years ago

https://github.com/benjamn/recast/blob/24ebc0e110aed78326fc1278d888912c4a53cb7b/package.json#L28

Now that esprima-fb is being deprecated (https://github.com/facebook/esprima/issues/111), will recast switch to regular Esprima instead?

benjamn commented 9 years ago

Yes, I think I would like to run the test suite using both https://github.com/jquery/esprima and https://github.com/marijnh/acorn. Any other suggestions?

mathiasbynens commented 9 years ago

There’s also https://github.com/eslint/espree.

danieljuhl commented 9 years ago

babel-core?

cpojer commented 9 years ago

I think https://github.com/babel/acorn-babel would be a good contender, despite the README saying it is not supported outside of babel. Babel is quickly becoming a transpiler standard.

fkling commented 9 years ago

@cpojer: I don't think that project is kept up-to-date anymore. The acorn fork/extension seems to be kept inside babel-code.

fkling commented 9 years ago

@benjamn: Is there any progress on supporting different parsers? If you are busy, maybe you could outline what needs to be done so that's easier for others to help?

benjamn commented 9 years ago

In principle, recast.parse(source, { esprima: require("any/parser/you/like") }) should work already for any sufficiently Esprima-like parser module (including Acorn AFAIK), so it should be just a matter of actually running tests using different parsers to provide more confidence there?

If you have time to take a look at the test runner to use other parsers, that would be great!

benjamn commented 9 years ago

Specifically, I would add other parsers to devDependencies in package.json, then allow the test suite to require a different parser based on an environment variable like process.env.PARSER_MODULE_ID, then add a npm script called test-all-parsers that invokes (at least the parser-related) tests for every parser. That comprehensive test should be what runs in Travis CI, though it's fine if the ordinary npm test command just uses the default parser.

This testing might reveal that more pretty-printer cases need to be added to lib/printer.js, but that work should be minimal.

nene commented 8 years ago

It's been a while and esprima-fb is still deprecated. Should Recast also be considered as deprecated?

Like, I'm trying to parse this code:

function fn({foo={}} = {}) { }

...and Recast simply blows up because esprima-fb is unable to handle it. Official Esprima parses it just fine though.

fkling commented 8 years ago

@nene: you can always pass another parser to recast via the esprima option. But I agree that recast should upgrade to another default parser or not provide a default at all (which would be a breaking change though).

nene commented 8 years ago

Ah... thanks. I wasn't aware of such an option... or really the existence of any options as these are only documented inside the source.

benjamn commented 8 years ago

@nene yeah, sorry about the lack of documentation. Recast is a long-running, surprisingly successful side project of mine, and while I'm happy to answer any and all questions about it, it's never going to be something I can polish and evangelize the same way I would a professional project. Knowing that I don't really have time to invest in documentation or a website creates pressure on me to keep the basic API as simple as I can, which seems like a virtue? I don't know. I wish I could work on it full time.

Your suspicion that Recast is "deprecated" because one of its dependencies is no longer maintained strikes me as a rather strange leap of reasoning. If you think a different parser would be more appropriate, feel free to submit a pull request to change the default!

benjamn commented 8 years ago

Open question: which parser should be the default? Is https://www.npmjs.com/package/babylon still as Babel-specific as it used to be? Does https://github.com/jquery/esprima support all the syntax you might want?

benjamn commented 8 years ago

One advantage of esprima-fb over esprima: it parses JSX syntax.

benjamn commented 8 years ago

Just released/published v0.11.0 with esprima as the default parser.

I'm not thrilled with this solution, because there are a lot of syntaxes that esprima does not yet support, such as JSX, decorators, and async functions.

For syntax completeness, it might make more sense to use babylon. Would anyone object to that?

benjamn commented 8 years ago

Another argument for using babylon as the default is that it's harder to configure appropriately than esprima, so there's more value in doing that configuration automatically. The upshot of that configurability is that we can be more selective about which syntax features are enabled.

fkling commented 8 years ago

For syntax completeness, it might make more sense to use babylon. Would anyone object to that?

Regarding completeness I agree. I'm a bit afraid of how babylon will deviate from the ESTree spec and well documented / communicated these deviations are. But I'm probably overreacting here.

benjamn commented 8 years ago

Would espree be more complete than esprima?

nene commented 8 years ago

@benjamn, big thanks for the release! Sorry for overreacting, I guess my frustration with this problem grew a bit too big and it came through in my comments.

benjamn commented 8 years ago

No worries, and thanks for understanding. Frustrated bug reports are much better than silence!

ariya commented 8 years ago

@benjamn Just the other day I was thinking of optionally support JSX syntax in Esprima :smile: I'll keep you posted!

baweaver commented 7 years ago

Another fun factor to consider is that esprima-fb brings some nice sized cruft along with it and they're not keen on cutting any new releases after deprecating it:

760K    ./node_modules/regexpu/node_modules/recast/node_modules/esprima-fb/test/test.js

Tried to get it pulled out here, but no luck: https://github.com/facebookarchive/esprima/pull/120