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

Speed improvement #53

Closed skaytes closed 9 years ago

skaytes commented 9 years ago

Explained in the commit comments.

zertosh commented 9 years ago

So It's actually much easier and faster to use acorn's traverser. I didn't make the switch because the docs make no mention as to what type of node a custom isRequire is allowed to return true for. You'd think "any node", but there's an implicit assumption that it's a CallExpression (because of the arguments.length any other node type would throw). So, not wanting to update the docs and bump major versions, I just forgot about it.

I don't know why you running your code in strict mode would cause node-detective to throw. I know node-detective itself can't run in strict mode - which is why it doesn't. Can you explain why to me? Or how I can test it?


diff --git a/index.js b/index.js
index f16132a3..3b2653d4 100644
--- a/index.js
+++ b/index.js
@@ -1,6 +1,7 @@
 var aparse = require('acorn').parse;
 var escodegen = require('escodegen');
 var defined = require('defined');
+var walk = require('acorn/dist/walk');

 var requireRe = /\brequire\b/;

@@ -38,11 +39,6 @@ var traverse = function (node, cb) {
     }
 };

-var walk = function (src, opts, cb) {
-    var ast = parse(src, opts);
-    traverse(ast, cb);
-};
-
 var exports = module.exports = function (src, opts) {
     return exports.find(src, opts).strings;
 };
@@ -54,11 +50,8 @@ exports.find = function (src, opts) {
     if (typeof src !== 'string') src = String(src);

     var isRequire = opts.isRequire || function (node) {
-        var c = node.callee;
-        return c
-            && node.type === 'CallExpression'
-            && c.type === 'Identifier'
-            && c.name === word
+        return node.callee.type === 'Identifier'
+            && node.callee.name === word
         ;
     }

@@ -68,17 +61,19 @@ exports.find = function (src, opts) {
     var wordRe = word === 'require' ? requireRe : RegExp('\\b' + word + '\\b');
     if (!wordRe.test(src)) return modules;

-    walk(src, opts.parse, function (node) {
-        if (!isRequire(node)) return;
-        if (node.arguments.length) {
-            if (node.arguments[0].type === 'Literal') {
-                modules.strings.push(node.arguments[0].value);
-            }
-            else {
-                modules.expressions.push(escodegen.generate(node.arguments[0]));
+    walk.simple(parse(src, opts.parse), {
+        CallExpression: function(node) {
+            if (!isRequire(node)) return;
+            if (node.arguments.length) {
+                if (node.arguments[0].type === 'Literal') {
+                    modules.strings.push(node.arguments[0].value);
+                }
+                else {
+                    modules.expressions.push(escodegen.generate(node.arguments[0]));
+                }
             }
+            if (opts.nodes) modules.nodes.push(node);
         }
-        if (opts.nodes) modules.nodes.push(node);
     });

     return modules;
skaytes commented 9 years ago

So running node-detective in strict mode isn't supported? If so then that's why it was failing because I have Node running with the global strict mode flag set. It works on my own scripts with the above commits though. Is there some other reason strict mode can't be supported?

Oh, and it isn't throwing explicitly, it's erroring due to the strict mode violation.

ghost commented 9 years ago

Global strict mode in node is a mistake that goes against how strict mode works. Don't use it and expect third-party code to work.

https://github.com/substack/node-mkdirp/issues/32#issuecomment-23590736

skaytes commented 9 years ago

Updated title from "Fix throwing in strict mode" since it could imply something was broken.

zertosh commented 9 years ago

I didn't know that flag existed. @bytewalk I'll make a PR with the above diff, but not as "Strict mode support" but rather as a speed improvement.

skaytes commented 9 years ago

I didn't check performance data, but I've changed the title again if that's what you were referring to.