Add option to use parse_dammit

[dependents]

Add option to use parse_dammit

[mrjoelkemp]

Hey @jimthedev. Thanks for contributing!

I find it odd that they opted for adding new parse alternatives instead of specifying a mode or something. In any case, it's a great idea and I'd totally accept an options parameter as the third argument to https://github.com/mrjoelkemp/node-precinct/blob/2f1188b31335412b57b2d028498b1d5faba4b111/index.js#L21. In there, you'd specify something like parseMethod: 'acorn_loose' or parseMethod: 'parse_dammit' with a default value of 'parse'.

In https://github.com/mrjoelkemp/node-precinct/blob/2f1188b31335412b57b2d028498b1d5faba4b111/index.js#L29, you'd then do something like acorn[options.parseMethod].

Would you like to submit a PR?

[mrjoelkemp]

Also, when you say "if one of the jspm modules has serious problems in a file with a .js extension.", what type of problems are we talking about?

Ignoring parse errors is another issue (perhaps it should be a separate ticket). I've always preferred strengthening the parser over swallowing errors. Particularly because if a user gets back an incomplete list of dependencies, they'll think the module is broken when the underlying parser could have been improved.

1. Out of curiosity, what are you using node-list-dependencies for exactly?
2. Once you clarify the "serious problems," we can see if the underlying detectives of precinct could be improved.

[jimthedev]

Yes! I'd be happy to. I also found their implementation odd. Do you want to make a call on whether to use loose or strict as the default. I'd imagine strict.

On Tue, Feb 3, 2015 at 5:13 PM, Joel Kemp [email protected]
wrote:

Hey @jimthedev. Thanks for contributing!
I find it odd that they opted for adding new parse alternatives instead of specifying a mode or something. In any case, it's a great idea and I'd totally accept an options parameter as the third argument to https://github.com/mrjoelkemp/node-precinct/blob/2f1188b31335412b57b2d028498b1d5faba4b111/index.js#L21. In there, you'd specify something like parseMethod: 'acorn_loose' or parseMethod: 'parse_dammit' with a default value of 'parse'.
In https://github.com/mrjoelkemp/node-precinct/blob/2f1188b31335412b57b2d028498b1d5faba4b111/index.js#L29, you'd then do something like acorn[options.parseMethod].

Would you like to submit a PR?

Reply to this email directly or view it on GitHub:
#11 (comment)

[mrjoelkemp]

I'm not sure what you mean by loose or strict. I thought the options were acorn_loose, parse_dammit, or just parse in terms of methods to call on acorn via the new parseMethod attribute of the options object you'll supply.

[jimthedev]

Edit: This can no longer be reproduced with these commands because ui-router updated my pull request, removing the problem file. The issue still exists, however.

Hi Joel,

I was on mobile before so that certainly wasn't my most clear and detailed reply. I'll try to do better here.

First off, my use case:

I am trying to use the node-list-dependencies package to list the dependencies in my app. Since node-list-dependencies uses node-precinct for parsing I figured it didn't make sense to put the issue in node-list-dependencies. Sorry for the confusion and lack of context. :)

Everything works great with the list-deps command until I introduce a specific jspm package. I think the best way to illustrate it is to give you the ability to reproduce my issue in a few commands:

sudo npm install -g jspm list-deps
mkdir jspm-list-dependencies-bug && cd $_
jspm init --yes && jspm install ionic=1.0.0-beta.13 && list-deps ./

So all we're really doing here is creating a new folder and jspm package with all of the default settings, installing the ionic jspm package and its dependencies and running list-deps on the resulting folder structure. I get the following error and stack trace:

---
 app/jspm_packages/github/angular-ui/[email protected]/config/jsdoc.js
---

/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:335
    throw err;
          ^
SyntaxError: Unexpected token (2:11)
    at raise (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:333:15)
    at unexpected (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1349:5)
    at semicolon (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1336:47)
    at parseExpressionStatement (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1786:5)
    at parseStatement (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1572:19)
    at parseBlock (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1809:18)
    at parseStatement (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1555:26)
    at parseTopLevel (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:1509:18)
    at Object.exports.parse (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/node_modules/acorn/acorn.js:48:12)
    at module.exports (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/node_modules/precinct/index.js:29:17)
    at printDependencies (/Users/jcummins/bin/npm/lib/node_modules/list-dependencies/bin/list-deps.js:39:22)

ui-router is one of ionic's dependencies which is why we're seeing it referenced in the stack trace.

The problem is that the jsdoc.js file in the ui-router repo isn't actually javascript. It is json! As far as I can tell it isn't even actually used in their project. They now use ng-docs instead of jsdoc3 so it is an orphan .js. It just hangs out in the directory structure and is invalid javascript. I am going to put in a PR there to fix it but there are 30 PRs and over 300 issues so it may take some time and would not get backported into old versions of ui-router on which ionic relies. I am glad the parser told me about this, but, I also wish there was a way for me to get around this while I go through the PR process.

So, bottom line, my hope is that we can add a way for the dependency list not to break every time someone upstream accidentally commits a .js file upstream that can't be parsed, especially if it isn't used anywhere.

I think the sane default is to keep the current 'strict' parsing functionality that errors when there is a serious parse error, but, to also offer node-list-dependencies or other packages the ability to explicitly set 'loose' parsing if they would prefer.

Now, just to clarify what I mean by loose and strict. I think based off of my original post you might have thought that loose and dammit are two different parsing methods, with a third being the current parsing method. This is not the case. There are only two parsing methods, one is acorn.parse (strict, you're currently using this) and one is acorn.parse_dammit (loose, I'd like to use). So to use your example, we'd only need to allow parseMethod: 'parse' and parseMethod: 'parse_dammit'.

Edit: This can no longer be reproduced with these commands because ui-router updated my pull request, removing the problem file. The issue still exists, however.

[mrjoelkemp]

Thanks a ton for clarifying @jimthedev. The explanation helped and parse_dammit is pretty awesome come to think of it.

list-dependencies would need a cli option to allow the user to specify the parse mode that results in the parseMode of precinct being set. In other modules, I've used https://github.com/tj/commander.js for setting cli options (ex: https://github.com/mrjoelkemp/node-dependents/blob/master/bin/dependencyLookup.js). This would then require a major bump of list-deps due to the breaking change in the cli's usage (no biggie).

To clarify, I'm thinking list-dependencies would support a --parse-mode option that is used as the value for precinct's parseMode.

Let me know if you feel comfortable making the changes and if you have any questions.

[mrjoelkemp]

Ping @jimthedev. Still interested in tackling this?

[jimthedev]

I am, just haven't been able to dedicate time to it yet. If you want to close this as a future Todo or want to tackle it then that'd be fine with me. I've just had other priorities that have taken over for the past two weeks.

[jimthedev]

@mrjoelkemp So, I was looking at acorn's source and it appears that acorn assumes that you've run acorn.parse before running acorn.parse_dammit. The author makes specific note that it should not be used as a first resort because there are cases where it will not perform as well as parse. For that reason it may be more prudent to set up the flag to be something like --allowFallbackParser or --allowEs6ParserFallback. I'm a big fan of how verbose this is but it would make it clear that this is a fallback and not parse mode.

[mrjoelkemp]

Thanks for the additional info @jimthedev! What about just calling parse_dammit internally on a parse exception? That way, parse is called first (as the acorn docs suggest), and the user doesn't have to think about fallbacks.