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

speed up by using acorn tokenizer when possible #73

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Nov 26, 2017

Add a findFast function that's exactly like detective.find, but doesn't parse the source. detective.find automatically uses findFast if possible.

we cannot do this if a custom isRequire function is provided, because that can check an arbitrarily complex callee. If a custom isRequire function is not provided we only have to check for a single token with name opts.word, so then we can look at only the token stream.

acorn's tokenizer ignores comments so things like require /* xyz */ ('abc') already work.

when opts.nodes is set, detected require calls are parsed using acorn.parseExpressionAt.

opts.fullParse can be set in order to always do a full parse. this can be useful if folks want to make sure that all syntax is correct.

I've so far found only one case where this doesn't work:

detective("(require)('abc')", { fullParse: true }) // → ['abc']
detective("(require)('abc')") // → []

may add a special case for that if there's nothing else.


e;

Before:

➜  node bench/detect.js
147

After:

➜  node bench/detect.js
66

Copy link
Member

@bcomnes bcomnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and the conflict is very simple to fix, just bumping the ECMA version. I'm not to confident on what the side effects of this change could be though.

@bcomnes
Copy link
Member

bcomnes commented Dec 25, 2017

Fixed the merge conflict

@goto-bus-stop
Copy link
Member Author

yeah not sure about side effects either, might try using it in some more projects first

@goto-bus-stop
Copy link
Member Author

goto-bus-stop commented May 11, 2018

Well that got a lot more complex to support the scope stuff introduced by #79 😆 it looks like it works tho, will do a major bump for this to be safe (+ the scope stuff may technically be breaking as well)

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

Successfully merging this pull request may close these issues.

None yet

2 participants