search

browserify / detective

Find all calls to require() no matter how deeply nested using a proper walk of the AST
Other
414 stars 60 forks source link

remove esprima-six, go back to esprima #30

Closed max-mapper closed 10 years ago

max-mapper commented 10 years ago

esprima-six introduces regressions that break the browserifying of modules that used to work before the switch away from esprima, e.g.

$ browserify -r escodegen
SyntaxError: Line 819: Unexpected string while parsing /Users/maxypoo/src/node_modules/escodegen/escodegen.js
$ browserify -r detective
SyntaxError: Line 1460: Unexpected string while parsing /Users/maxypoo/src/node_modules/detective/node_modules/escodegen/escodegen.js
Error: Parsing file /tmp/glsl-parser11424-6356-1jwtxly/node_modules/glsl-parser/lib/index.js: Line 73: Comprehension Error

as stated by @thlorenz:

< thlorenz> better to have stable ES5 parsing than ES6 features (especially if there is always es6ify)

so this PR reverts back to esprima

thlorenz commented 10 years ago

:+1: + :100:

hughsk commented 10 years ago

:+1: :+1: :+1:

ghost commented 10 years ago

Merged in 3.0.0 but there are several other modules that use esprima-six in the browserify pipeline. These other modules will need to be downgraded to use esprima instead of esprima-six:

We should also get these modules upgraded before bumping the version in browsericy core. This will be a major version upgrade for browserify.

ghost commented 10 years ago
hughsk commented 10 years ago

@substack, just sent a PR to derequire to downgrade (calvinmetcalf/derequire#10), do you reckon this'll be good to go once that's merged? :)

andreypopp commented 10 years ago

You can consider using esprima-fb which is used by facebook in their es6-to-es5 code transform. It is also used by React's JSX tool. I think these validates that this parser is mature and supports es6.

max-mapper commented 10 years ago

it worries me that esprima, esprima-six and esprima-fb all have the exact same readme and neither of the forks document what they changed

andreypopp commented 10 years ago

@maxogden yeah, that's an issue.

I've checked a few examples which fails with esprima-six and it works ok with esprima-fb. I'm using esprima-fb myself for es6-to-es5 transforms and haven't seen any problems with it.

I've also submitted a series of PRs for modules used in browserify deps (and their deps) which were depended on esprima-six.

max-mapper commented 10 years ago

OK i tried esprima-fb in place of esprima-six and it passed my test cases as well as the detective tests so I think @substack is gonna switch to esprima-fb everywhere that esprima-six is used