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

Support minification of $routeProvider resolve functions #35

Closed
Siyfion opened this issue May 24, 2013 · 39 comments
Closed

Support minification of $routeProvider resolve functions #35

Siyfion opened this issue May 24, 2013 · 39 comments
Milestone

Comments

@Siyfion
Copy link

Siyfion commented May 24, 2013

At the moment the only part of my code that doesn't minify properly are my resolve functions in my $routeProvider.when() statements.

It would be nice to see these supported. 👍

@ffesseler ffesseler mentioned this issue May 30, 2013
Closed
@btford
Copy link
Owner

btford commented May 30, 2013

+1; I'm currently re-writing ngmin to use Astral, but this will be the next feature I add.

@meenie
Copy link

meenie commented Jun 4, 2013

I just realised this myself about 30 mins ago and had to change all of our resolve functions heh.

@btford, thanks a ton for your work on this, I really appreciate it 👍

@spacenick
Copy link

That would be dope for "any" resolve actually! (I'm using ui-router and $stateProvider for my routes)
Cheers @btford this tool is insane.

@lpaulger
Copy link

+1

2 similar comments
@mlegenhausen
Copy link

+1

@thesamet
Copy link

thesamet commented Jul 1, 2013

+1

@btford
Copy link
Owner

btford commented Jul 1, 2013

Added a test here: https://github.com/btford/ngmin/blob/master/test/route-provider.js#L24

If you have any other cases, please send a PR. Implementation to follow soon.

@btford
Copy link
Owner

btford commented Jul 2, 2013

Closed via btford/astral-angular-annotate@e157310

Reinstall ngmin to get the latest relevant dependency with the fix.

@btford btford closed this as completed Jul 2, 2013
@Siyfion
Copy link
Author

Siyfion commented Jul 2, 2013

@btford Is this worthy of a up-rev? That way we can just update from bower. ❓

@btford
Copy link
Owner

btford commented Jul 2, 2013

You don't update ngmin via bower.

@Siyfion
Copy link
Author

Siyfion commented Jul 2, 2013

Eak, npm even :/

Sent from Mailbox for iPhone

On Tue, Jul 2, 2013 at 4:47 PM, Brian Ford notifications@github.com
wrote:

You don't update ngmin via bower.

Reply to this email directly or view it on GitHub:
#35 (comment)

@btford
Copy link
Owner

btford commented Jul 2, 2013

If you update via npm you should get the new updated dependencies. There's no need to bump the version.

@thesamet
Copy link

thesamet commented Jul 3, 2013

The test you referred to (https://github.com/btford/ngmin/blob/master/test/route-provider.js#L24) does not handle resolve functions.

I have added an example of (a failing) test case:

thesamet@6171c7b

@Siyfion
Copy link
Author

Siyfion commented Jul 23, 2013

@btford And I remember what I meant now, grunt-ngmin is currently pulling ngmin in for me, however it's pulling in v0.4.0 which obviously doesn't have this fix (as yet). Is it possible we could have an interim update to grunt-ngmin?

@danieljsinclair
Copy link

I could do with this

@mlegenhausen
Copy link

There is no support for the resolve object. I added it to the astral annotations.

btford/astral-angular-annotate#2

@homburg
Copy link

homburg commented Aug 12, 2013

This issue does not seem to be fixed yet. See #35 (comment)

@mlegenhausen
Copy link

It isn't. It only works for inline controller definition as shown in the tests.

@homburg
Copy link

homburg commented Aug 12, 2013

Are there plans for solving this or including your pull request?

@mlegenhausen
Copy link

I haven't get any feedback yet. But you can use my fork and install it via the git url.

@homburg
Copy link

homburg commented Aug 12, 2013

Thank you. I have added your fork to my npm dependencies and it solves the
issue perfectly.

@mboudreau
Copy link

+1. Please fix this. This is completely stopping us from a production build.

@btford
Copy link
Owner

btford commented Sep 26, 2013

This is completely stopping us from a production build.

You do know that you can add the annotation yourself, right? There's no reason this should ever hold up anything.

@mlegenhausen
Copy link

@mboudreau For now modify your package.json as follows:

devDependencies: {
  "astral-angular-annotate": "git+https://github.com/werk85/astral-angular-annotate.git"
  ...
}

and your problems are solved until the fix is validated and accepted by @btford.

@mboudreau
Copy link

I tried that without success. It seems that the dependency doesn't seem to
override the ngmin dependency. Am I missing something?
On Sep 26, 2013 8:21 PM, "Malte Legenhausen" notifications@github.com
wrote:

@mboudreau https://github.com/mboudreau For now modify your package.jsonas follows:

devDependencies: {
"astral-angular-annotate": "git+https://github.com/werk85/astral-angular-annotate.git"
...
}

and your problems are solved until the fix is validated and accepted by
@btford https://github.com/btford.


Reply to this email directly or view it on GitHubhttps://github.com//issues/35#issuecomment-25157096
.

@mlegenhausen
Copy link

Uninstall grunt-ngmin or ngmin from your node_modules and install it again.

@mboudreau
Copy link

Awesome. Let me try that. Thanks bud.
On Sep 26, 2013 8:45 PM, "Malte Legenhausen" notifications@github.com
wrote:

Uninstall grunt-ngmin or ngmin from your node_modules and install it
again.


Reply to this email directly or view it on GitHubhttps://github.com//issues/35#issuecomment-25158310
.

@mboudreau
Copy link

Yeah, no dice, still not working even after uninstall, re-installing
astral-angular-annotate, and installing ngmin and grunt-ngmin afterwards...

On Thu, Sep 26, 2013 at 9:00 PM, Michel Boudreau
michelboudreau@gmail.comwrote:

Awesome. Let me try that. Thanks bud.
On Sep 26, 2013 8:45 PM, "Malte Legenhausen" notifications@github.com
wrote:

Uninstall grunt-ngmin or ngmin from your node_modules and install it
again.


Reply to this email directly or view it on GitHubhttps://github.com//issues/35#issuecomment-25158310
.

"If at first you don't succeed, use a bigger hammer." - Unofficial motto of
the Royal Electrical and Mechanical Engineers

@tlvince
Copy link

tlvince commented Oct 11, 2013

To confirm, when this lands, will it support ui-router $stateProvider controllers?

@paulmelnikow
Copy link

+1

@mlegenhausen
Copy link

@tlvince I don't think so. You will need to add your own annotation rules.

I think the best way would be a plugable annotation system, cause there are many more places where you need this stuff.

@szimek
Copy link

szimek commented Nov 20, 2013

Is it already implemented in some unreleased version of ngmin or astral-angular-annotate?

@mlegenhausen
Copy link

@szimek add the dependency manually to your dependency list. Then ngmin will pick up the new one.

@mlegenhausen
Copy link

I think the main release this doesn't get resolved is that the annotation system needs to be more adaptable. There are so many other libraries out there that would benefit from annotation support e.g. ui-router. So ngmin should not be responsible for the minification support for all the libraries but should offer an extensible system.

@szimek
Copy link

szimek commented Nov 20, 2013

@mlegenhausen I can agree with it, but it would be also great if the current non-extensible version of ngmin supported at least pure angular out of the box. It's already fixed and only a matter of merging a PR and releasing a new version. It doesn't conflict in any way with rewriting ngmin to support some kind of plugin system in the future.

@mlegenhausen
Copy link

Cause ngRoute is not a core component anymore the next step needs to be an extensible system. If you want to keep ngmin for the core this should not be resolved.

Making ngmin extensible shouldn't be a big problem. Maybe I find some time to solve this.

@szimek
Copy link

szimek commented Nov 20, 2013

Right, I forgot ngRoute was removed from core in 1.2.0.

@mlegenhausen
Copy link

Done. Look at my pull request #61 and vote for it ;)

@eddiemonge
Copy link
Collaborator

Please try https://github.com/olov/ng-annotate. ngmin is now deprecated: #93

If your issue isn't resolved there please open an issue at https://github.com/olov/ng-annotate/issues

If you really want ngmin to fix this issue, feel free to fork it and use that.

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