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

Dynamic Analysis #44

Closed
btford opened this issue Jul 29, 2013 · 10 comments
Closed

Dynamic Analysis #44

btford opened this issue Jul 29, 2013 · 10 comments

Comments

@btford
Copy link
Owner

btford commented Jul 29, 2013

No description provided.

@btford
Copy link
Owner Author

btford commented Jul 31, 2013

Proof of concept:

var fs = require('fs');

var js = fs.readFileSync('./test/basic.js', 'utf8');

var angular = {
  module: function () {
    return {
      controller: function (name, stuff) {
        if (stuff instanceof Array) {
          return;
        }

        var original = stuff.toString();

        var annotated = '[' +
          ___annotate(stuff).
            map(function (arg) {
              return "'" + arg + "'";
            }).
            concat([
              original.toString()
            ]).
            join(', ') +
          ']';

        js = js.replace(original, annotated);
      }
    }
  }
};

var ___annotate = (function () {

  'use strict';

  var FN_ARGS = /^function\s*[^\(]*\(\s*([^\)]*)\)/m;
  var FN_ARG_SPLIT = /,/;
  var FN_ARG = /^\s*(_?)(\S+?)\1\s*$/;
  var STRIP_COMMENTS = /((\/\/.*$)|(\/\*[\s\S]*?\*\/))/mg;

  return function annotate (fn) {
    var $inject,
        fnText,
        argDecl,
        last;

    if (typeof fn == 'function') {
      if (!($inject = fn.$inject)) {
        $inject = [];
        fnText = fn.toString().replace(STRIP_COMMENTS, '');
        argDecl = fnText.match(FN_ARGS);
        argDecl[1].split(FN_ARG_SPLIT).forEach(function(arg) {
          arg.replace(FN_ARG, function(all, underscore, name) {
            $inject.push(name);
          });
        });
      }
    } else if (fn instanceof Array) {
      last = fn.length - 1;
      $inject = fn.slice(0, last);
    }
    return $inject;
  }

}());


eval(js);

console.log(js);

Directive controllers will obviously be a tricky case.

@gsklee
Copy link

gsklee commented Aug 2, 2013

Can't this be done using Esprima (like what you are doing now via Astral)? Just being curious.

@btford
Copy link
Owner Author

btford commented Aug 2, 2013

No, that's static analysis. I could write an interpreter for the AST, but at that point I'd be implementing my own eval.

@gsklee
Copy link

gsklee commented Aug 8, 2013

👍 Great information!

@jcw
Copy link

jcw commented Aug 28, 2013

How about adding a special first function argument in each function definition which needs to be DI-protected, so that:

... function($DI$, $scope, blah) { ... } ...

gets transformed into:

... ['$DI$', '$scope', 'blah', function($DI$, $scope, blah) { ... }] ...

With $DI$ being an unused dummy argument (its name and value don't really matter).

Maybe that special argument could even be dropped during pre-processing (might be risky, since it affects the semantics):

... ['$scope', 'blah', function($scope, blah) { ... }] ...

Would something like this be feasible?

@btford
Copy link
Owner Author

btford commented Aug 28, 2013

comment annotations would be better. I don't want any annotations though. I want it to be completely automatic.

@jcw
Copy link

jcw commented Aug 28, 2013

I still think this approach would go a long way - it'd be a simple token stream transformation with a 3-token sliding window.

@jcw
Copy link

jcw commented Aug 28, 2013

Note that comment annotations are not so simple with CoffeeScript, which currently strips most comments.

@davegurnell
Copy link

I support @jcw's suggestion adding some kind of explicit macro annotation.

Without proper static analysis, ngmin is never going to be able to catch all situations in which DI annotations are needed. In fact, I suspect Javascript is too flexible a language for static analysis to be completely possible.

Personally I'm not a fan of @jcw's suggestion to add an argument (sorry - no offence meant). Aesthetically speaking, adding an argument looks like it's altering the semantics of the function being annotated. There are also precedents for this kind of thing that have been set in other languages.

Astral/esprima are essentially a macro system for Javascript. The idiomatic way of doing this in a language with a built-in macro system (Scheme, Clojure, ands Scala to name a few) would be to create a function-like form like this:

DI(function($scope, myService) {
  // ...
})

This would be easy to match in the AST, and you could trivially produce the equivalent array annotation form:

[ '$scope', 'myService', function($scope, myService) {
  // ...
} ]

Note that I'm not recommending using "DI" as the macro name - unless you're want to manually check to avoid naming collusions with existing variables (aka "maintaining hygiene" in macro world), you'll want a name that is unlikely to collide with identifiers in the user's code - "$di" or something like that.

I, personally, would feel more comfortable using an explicit annotation system like this than automatic detection for the reasons I described above - ngmin's automatic detection is great, but even with the best will in the world it can never be perfect. Explicit annotation also opens up a more friendly syntax to Coffeescript users, e.g.:

DI ($scope, myService) ->
  # ...

I hope this sells the idea of explicit annotations a bit more strongly. I'd be more than happy to work on a contribution along these lines if it's of interest. It would also make sense to add a few options to the configuration for ngmin and grunt-ngmin to switch explicit and automatic detection on/off and customise the name of the "DI" macro.

@jcw
Copy link

jcw commented Oct 1, 2013

adding an argument looks like it's altering the semantics of the function being annotated

IMHO, with DI, function arguments have no relevant order, they act as "import names" to hook into some external context - in that light, a $DI$ style marker to me would tag the function as being DI-based.

But hey, so would your novel explicit DI (...) -> ... annotation style.

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

No branches or pull requests

5 participants