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

Issues when minified. #29

Closed
Darrenthebrit opened this issue May 5, 2014 · 5 comments
Closed

Issues when minified. #29

Darrenthebrit opened this issue May 5, 2014 · 5 comments

Comments

@Darrenthebrit
Copy link

Hi,

We are utilizing your code in one of our projects. When published, all of our JS is minified (thanks to bundling and minification in .Net)

What we found was that our JS would error out with an unknown provider.

After picking through all our code we found that the angular-scroll source wasn't really written for minification in mind.

I.E where you have

factory('polyfill', function($window) { ..... });

we changed it to

factory('polyfill', ['$window', function($window) { ....}]);

We did this for all DI methods and it's resolved our issue.

Essentially, because minification changes all the parameter names Angular can't match the new provider parameter name with a known provider.

Not sure if you want to put the fix in your codebase..

@oblador
Copy link
Owner

oblador commented May 5, 2014

Dupe of https://github.com/durated/angular-scroll/pull/11.

In my point of view it's not the library thats not minification compatible, it's your minifier that's not angular compatible. I'd suggest to either:

  1. use the minified version
  2. use ngmin or some variation

@Darrenthebrit
Copy link
Author

That's your call :-) just passing on my findings. Personally I believe the library, any library like this, shouldn't have to rely on certain minifiers. Angular, in this case, provide a mechanism for the angular code to be safe regardless of which minifier is used.

Also, in a development environment the minified version of any library isn't much use should it ever need modifying

Like I said, your call. A small change means others won't run into the same issue :-)

@oblador
Copy link
Owner

oblador commented May 5, 2014

Well I do serve a production version (that's even the only version you can download in the release section) that is minified and uses the ['$window', function($window)] syntax. If you want to compile it yourself then that's up to you, but I don't see the point unless you only want to use parts of the library (which is a valid use case in larger libraries, but maybe not this one although I made it possible to do so).

In the case of a development environment, your setup surely must support using different sources depending on environment? (For example serving Angular or jQuery from a CDN in production is a best practice that (most?) people use). If not then I also include a minification map which should be helpful for debugging minified scripts.

@Darrenthebrit
Copy link
Author

We don't use any of the public CDN in production - not everybody does for various reasons.

In this particular setup we're using .net for the web api and delivery of views for the angular routes .net also takes care of our bundling. In terms of JavaScript, anything with .min in the filename is not used by .net when debugging. So for that reason, as well as the unreadablity of a minified file we use the un-minified ones - they get minified and bundled when publishing.

All I was doing was pointing out that this particular library was having issues with the minification process. We have utilized , perhaps half a dozen other libraries and none have followed the unsafe way (in terms of minification) to declare DI.

I have resolved the issue on our end and haven't had to resort to adding other libraries to perform minification for the odd script that doesn't follow our other practices.

It's a great library you have put together, like I said, just letting you know what I found.

oblador added a commit that referenced this issue May 6, 2014
@oblador
Copy link
Owner

oblador commented May 6, 2014

I changed so that the angular-scroll.js is also preminified by ngmin but keep the source as I like it :-) This should probably solve your problem unless you cherry pick certain directives/services/etc.

@oblador oblador closed this as completed May 6, 2014
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

2 participants