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

adding support to provide custom parse opts and stop esprima from throwing on script level returns #27

Closed thlorenz closed 10 years ago

thlorenz commented 10 years ago

This PR is in two parts.

I put them separately if for whatever reason you have a problem with the 2nd one. The most important is the first commit. However the current implementation has incorrect location information for the first line which is a problem if you want to use detective in order to replace requires instead of just finding them.

tolerant: true stops esprima from throwing and is a better solution IMO than wrapping the code inside a function.

  1. adding support to provide custom parse opts
    • necessary if you want parser to include extra info like location info in the AST
  2. esprima tolerant: true and removing wrapper
    • tolerant parsing does not throw on script level returns
    • enforcing this option for parse makes the function wrapper no longer needed
    • location info for first line is now correct (see test/parseopts.js)

      Note

0.6 currently fails on travis (certificate error) if you want I can change it to support 0.8 and 0.10 instead and add that to this PR.

defunctzombie commented 10 years ago

does tolerant change any other behavior other than script level returns?

LGTM as long as we understand any behavior changes.

thlorenz commented 10 years ago

Quoting from the docs:

tolerant: attempts to continue parsing when an error is encountered

So a more tolerant/stable parse is what we'd want right? All we are trying to do is find require calls, not validate JavaScript.

@ariya will know more, but as far as I can tell, it fixes the problems that required a function wrapper in the past and yes, mostly that's related to script level returns.

defunctzombie commented 10 years ago

This quote seems to imply that it will ignore all script errors. I guess that is reasonable for this module since it isn't actually interested in validating the JS. Tho maybe a script error could lead to improper detection of a require?

thlorenz commented 10 years ago

From what I understand, that's not how it works. It still has to understand the code. In this example it finds a problem/violation and will now throw or just add it to errors array if tolerant: true.

So all tolerant: true does is turn off throwing on ECMA spec violations, but if you have totally invalid code where it may improperly detect requires, it'll still blow up.

thlorenz commented 10 years ago

@substack would really like to see this land since I don't wanna publish browserify-shim while still depending on a github fork. Is there anything I should change so you can pull this?

ghost commented 10 years ago

merged in 2.3.0

thlorenz commented 10 years ago

Awesome, thanks!