Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False 'unused dependency' for asyncawait. #259

Closed
jakeleventhal opened this issue Jul 29, 2018 · 8 comments
Closed

False 'unused dependency' for asyncawait. #259

jakeleventhal opened this issue Jul 29, 2018 · 8 comments

Comments

@jakeleventhal
Copy link

When importing asyncawait as such:
const async = require('asyncawait').async; const await = require('asyncawait').await;

It says that asyncawait is an unused dependency.

mnkhouri added a commit that referenced this issue Aug 10, 2018
Updates the issue templates to collect version information and example code. I failed to reproduce an issue today (#259), so it seems helpful to collect this information in general
mnkhouri added a commit that referenced this issue Aug 10, 2018
…e code. I failed to reproduce an issue today (#259), so it seems helpful to collect this information in general
@mnkhouri
Copy link
Member

Our parser (Babylon) is throwing an error when it encounters const await because await is a reserved word.

I can't currently think of a way to fix this issue... Here are some things that won't work:

  • There is no option to "allow reserved words" in Babylon (some other JS parsers, like Acorn, seem to have such an option).
  • I don't forsee us swapping out Babylon for Acorn or another parser

@jakeleventhal
Copy link
Author

What if you built a step at the end that basically had a list of "packages to check". For each of those, if it finds, that it is unused, then it greps the project to quickly see if there is matching syntax (as i have shown in the issue ticket) and will remove it from the list of unused dependencies accordingly if it finds a match?

@mnkhouri
Copy link
Member

Neat idea! We would need to handle lots of edge cases though, since there's lots of possible syntaxes that can be used for importing a module (e.g. let async=require( "asyncawait" ) is also valid). It's nice to have the parser handle all those edge cases for us, so I've created a new issue (#269) to revisit whether Acorn might be usable instead of Babylon.

I'm starting to think that the root issue here is that the Babylon parser has a fixed list of reserved words, no matter which ES version it is parsing. So even though using await as a variable name is valid before ES6, Babylon will throw an error (and then we don't get any information about the modules used in that particular file).

On the other hand, the Acorn parser can be configured to parse in ES5 mode, and probably would parse your file OK.

I'll do some investigation in the next couple weeks for #269

btw thanks for the issue!

@jakeleventhal
Copy link
Author

of course, and thanks for actually maintaining this project lol. I think it wouldn't be too hard to have a list of reserved words like i mentioned and then just check the lines that appear in. if there is any line that has both
require followed by <special word>, then it will untag the dependency as 'unused'

I think that could be accomplished with some simple regexing and not require switchign from babylon to acorn

@LinusU
Copy link
Member

LinusU commented Aug 10, 2018

This was a very interesting experience: 😆

screen shot 2018-08-10 at 11 32 41

Didn't think that would be valid.

Anyhow, I think that doing the regexing dance is bad because of a few reasons:

  1. It will probably increase running time by quite much
  2. It could have false positives
  3. It shouldn't be needed

Switching to a better parser seems like the right thing to do.

no matter which ES version it is parsing.

I don't think that there are different versions though? My test above was in Node.js 10 which supports ES2018 🤔

I think that const async = ... should be valid in all JavaScript contexts.

@mnkhouri
Copy link
Member

Interesting test! Node seems to treat some reserved words different than others (this is with v8):

image

I don't think that there are different versions though?

I think at some point, before classes were introduced to ES, you would have been able to say var class = 42 in Node. Doing that in a modern Node version will throw a SyntaxError. Node versions don't map directly to ES versions, but there's definitely some change over time

Switching to a better parser seems like the right thing to do.

I'll add some more thoughts in #269

@LinusU
Copy link
Member

LinusU commented Aug 10, 2018

I think at some point, before classes were introduced to ES, you would have been able to say[...]

I think that class have been a reserved keyword since the beginning of JavaScript, which is why Node (or other runtimes) won't accept that as a variable name.

async and await on the other hand was added later to the spec, after the initial release, and thus had to be backwards compatible.

@jakeleventhal
Copy link
Author

I agree, I think this issue can be closed and that the proper solution is to switch the parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants