fix(ngMock): preventing minifier from breaking ngMock's inject

[kpolitowicz]

see #3531

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[kpolitowicz]

Patched on master, but 1.0.x also needs that (using 1.0.7 in production)

Edit: just fyi - I didn't know how/if to write a spec for this, but I made sure grunt test:unit passes.

[petebacondarwin]

We probably need to do this one too: https://github.com/kpolitowicz/angular.js/blob/85d9e7bcf3ca22488cb860c1fbfdbee5dfd83119/src/ngMock/angular-mocks.js#L1669

[pkozlowski-opensource]

Sorry for a stupid question, but why do we need to minify mocks? Those are used for testing only after all...

[petebacondarwin]

In case it is getting auto minified by the Rails pipeline

[kpolitowicz]

First of all, it may be jasmine-rails gem's fault, but I could not get my jasmine-specs.js uncompressed, no matter what Rails config options I used. It may be very version-specific as I find it strange nobody had this problem before.
But secondly, it makes sense to test your JS in as-close-to-production environment. So I'm not actually sure whether to appreciate the way jasmine minifies the JS for unit tests or call it a bug. :)

[pkozlowski-opensource]

@kpolitowicz unfortunately I'm not too familiar with Rails env but IMO minifing tests and mocks doesn't make much sense - it is just unnecessary work. The point is that we shouldn't deploy unit tests / mocks to production. So while testing your application code in the minified form makes sense I would rather spent time configuring testing env instead of forcing minification-safe annotations on mocks and tests.

But this is just my point of view of course.

[kpolitowicz]

@pkozlowski-opensource The trouble with "configuring testing env" is that if only one person wants to minify their JS unit tests, it is the angular-mocks.js code that breaks, not the app code. Unless you go to the lengths to only minify the code of your app and just use mocks (and other test files) unminified, which is what you meant, I think.

However, it is the way jasmine-rails prepares the test env for jasmine run - it just goes thru all your app, specs, helpers, and support files and minifies it using the Rails asset pipeline into one jasmine-specs.js to include in runner.html. At this moment I'm unsure if going with 2 files: jasmine-app.js and jasmine-spec.js (this one being always just concatenated, not minified) is a good idea - this is the question to jasmine-rails maintainers. Anyway, there are some things in Rails that happen naturally, by design, and this is one of them. And for my particular code base I'd needed a PR to angularjs or jasmine-rails - and it was easier to patch one JS file in this case.

Edit: after pondering on it for a while, I really do think that - no matter what - the JS code should be written in a consistent way. If you write a code (e.g. angularjs) that breaks when industry-standard tools (e.g. minifying tools) are used and then you provide a solution (i.e. annotation technique when injecting modules), your solution should be applied throughout your code. Because people will be expecting this solution to work the same way everywhere. IMHO :)

[kpolitowicz]

@petebacondarwin PR updated with ngMockE2E :)

[kpolitowicz]

@pkozlowski-opensource Just out of curiosity - is there a reason (besides wasting maintainer's time on unneeded changes :)) why minify-safe notation should not be used by default (only if needed)? I'm thinking performance or something? Because code readability doesn't suffer that much (everybody is used to it already, as a necessity in production code).

[pkozlowski-opensource]

@kpolitowicz yes, this is only about spending time on sth that is not needed.

[kpolitowicz]

Actually, there is one more thing that needs to be patched, if you use $timeout in your code:

$provide.decorator('$timeout', ['$delegate', '$browser', function($delegate, $browser) {
    $delegate.flush = function() {
      $browser.defer.flush();
    };
    return $delegate;
  }]);

This fails, even if you don't inject $timeout into your specs - it is enough to have it in a tested code (e.g. directive).

Let me know if you need a PR for this - or you think that angular-mocks.js should not ever be minified and will not use my patch anyway.

[btford]

I vote against this.

We do not provide a minified ngMock, and I don't want to start a precedent for it. I son't think this is a common enough issue to be worth it. This seems like more of an issue to be resolved with your environment than with ngMock.

Still, you're welcome to maintain a fork with the annotations for your own use, @kpolitowicz. Alternatively, you might be interested in my tool, ngmin, which attempts to automagically add the annotations.