LinkedInAttic / inject

AMD and CJS dependency management in the browser
http://www.injectjs.com
Other
464 stars 48 forks source link

Issues parsing coffee-script.js and xregexp-all.js #216

Closed brianmhunt closed 11 years ago

brianmhunt commented 11 years ago

I encounter a couple parsing errors when Inject parses the above scripts, which you can find here:

Perhaps the issue is in the link.js parser. You can forgive it for XRegExp, because it's madness in there :), but the parser dies on an innocuous line of coffee-script.

Sorry I do not have any links to test cases; I don't know the Inject API well enough yet to dive into a test case in jsFiddle/jsBin. If I get a chance I will try to put up tests.

If making link.js work proves challenging, a full javascript parser may be an alternative. I have used the full javascript parsers Acorn and Esprima, and they did not seem to encounter issues of the sort noted above. Acorn is only 28k when minified, whereas Esprima is used by RequireJS.

Performance-wise, I expect Acorn is the fastest full javascript parser written in javascript right now, edging out Esprima in the performance comparisons I have seen. Some incomplete parsers such as link.js could be considerably faster, though it could be at the expense of coverage.

Incidentally, here is how I detect dependencies in my YAMDO project, which converts into javascript something like this:

    /* called by way of traverse.forEach( ... )† on the AST 
       returned by Acorn  */
    return this.at_node_hook(function(node) {
      var args, callee, dependency_array, _ref;
      if (!(node.type === 'CallExpression' 
         && ((_ref = node.callee.name) === 'define' || _ref === 'require')
         && node["arguments"].length >= 1)) {
        return; // this is not a define() or require() call
      }
      callee = node.callee;
      args = node["arguments"];
      if (callee.name === 'require' && args.length === 1 
          && args[0].type === 'Literal') {
        // require("STRING")
        seen[args[0].value] = _.extend(node, {
          _origin: origin
        });
        return;
      }
      if (args[0].type === 'ArrayExpression') {
        // define([DEPS],...) or require([DEPS])
        dependency_array = args[0];
      } else if (callee.name === 'define' 
          && args.length >= 2 && args[1].type === 'ArrayExpression') {
        // define('id', [DEPS], ...)
        dependency_array = args[1];
      }
      if (!dependency_array) {
        return;
      }
      return _.each(dependency_array.elements, function(element) {
        // "_" being from lodash ¥
        if (element.type !== "Literal") {
          log.warn("dependency was not a literal; at line ",
                       element.loc.start.line);
          return;
        }
        return seen[element.value] = _.extend(element, {
          _origin: origin
        });
      });
    });

traverse ¥ lodash

I hope these parsing issues are easy to test and sort out! :)

jakobo commented 11 years ago

I would start with the link.js folks. They are super responsive when there are parse issues.

https://github.com/sebmarkbage/link.js On Jan 9, 2013 12:33 PM, "Brian M Hunt" notifications@github.com wrote:

I encounter a couple parsing errors when Inject parses the above scripts, which you can find here:

  • coffee-scripthttps://raw.github.com/jashkenas/coffee-script/1.4.0/extras/coffee-script.js
  • XRegExphttps://raw.github.com/brianmhunt/xregexp/master/xregexp-all.js

Perhaps the issue is in the link.js parser. You can forgive it for XRegExp, because it's madness in there :), but the parser dies on an innocuous line of coffee-script.

Sorry I do not have any links to test cases; I don't know the Inject API well enough yet to dive into a test case in jsFiddle/jsBin. If I get a chance I will try to put up tests.

If making link.js work proves challenging, a full javascript parser may be an alternative. I have used the full javascript parsers Acornhttps://github.com/marijnh/acornand Esprima http://esprima.org/, and they did not seem to encounter issues of the sort noted above. Acorn is only 28k when minified, whereas Esprima is used by RequireJS http://requirejs.org/.

Performance-wise, I expect Acorn is the fastest full javascript parser written in javascript right now, edging out Esprima in the performance comparisons I have seen. Some incomplete parsers such as link.js could be considerably faster, though it could be at the expense of coverage.

Incidentally, here is how I detect dependencies in my YAMDOhttps://github.com/brianmhunt/yamdo/blob/master/lib/Script.coffee#L143project, which converts into javascript something like this:

_register_depends_at_node: function() { var origin, seen, _this = this; seen = this._depends; origin = this.origin; /* snip - stuff about dependency options */

/* called by way of traverse.forEach( ... )† on the AST        returned by Acorn  */
return this.at_node_hook(function(node) {
  var args, callee, dependency_array, _ref;
  if (!(node.type === 'CallExpression'
     && ((_ref = node.callee.name) === 'define' || _ref === 'require')
     && node["arguments"].length >= 1)) {
    return; // this is not a define() or require() call
  }
  callee = node.callee;
  args = node["arguments"];
  if (callee.name === 'require' && args.length === 1
      && args[0].type === 'Literal') {
    // require("STRING")
    seen[args[0].value] = _.extend(node, {
      _origin: origin
    });
    return;
  }
  if (args[0].type === 'ArrayExpression') {
    // define([DEPS],...) or require([DEPS])
    dependency_array = args[0];
  } else if (callee.name === 'define'
      && args.length >= 2 && args[1].type === 'ArrayExpression') {
    // define('id', [DEPS], ...)
    dependency_array = args[1];
  }
  if (!dependency_array) {
    return;
  }
  return _.each(dependency_array.elements, function(element) {
    // "_" being from lodash ¥
    if (element.type !== "Literal") {
      log.warn("dependency was not a literal; at line ",
                   element.loc.start.line);
      return;
    }
    return seen[element.value] = _.extend(element, {
      _origin: origin
    });
  });
});

}

† traverse https://npmjs.org/package/traverse ¥ lodash http://lodash.com/

I hope these parsing issues are easy to test and sort out! :)

— Reply to this email directly or view it on GitHubhttps://github.com/linkedin/inject/issues/216.

brianmhunt commented 11 years ago

Oh wonderful. I didn't realize it had a whole project! :+1: I'll go report there. Thanks.

See Issues

jakobo commented 11 years ago

Thanks for linking. We'll follow those issues here. @sebmarkbage has been awesome with fixes and support.

sebmarkbage commented 11 years ago

Thanks! Fixed in link.js

jakobo commented 11 years ago

Closing this, will track as a library upgrade in 0.4.2