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

Excessive debug log messages on changes to $scope.markers #829

Closed
MattSidor opened this issue Jun 30, 2015 · 23 comments
Closed

Excessive debug log messages on changes to $scope.markers #829

MattSidor opened this issue Jun 30, 2015 · 23 comments
Milestone

Comments

@MattSidor
Copy link

When the user hovers over a marker, I am changing the marker size and anchor:

    function growMarker(markerName) {
        //double marker icon size and slightly adjust anchor so it stays centered
        $scope.markers[markerName].icon.iconSize = [24,24];
        $scope.markers[markerName].icon.iconAnchor = [11,11];
    }
    function shrinkMarker(markerName) {
        //restore marker icon to original settings
        $scope.markers[markerName].icon.iconSize = [17,17];
        $scope.markers[markerName].icon.iconAnchor = [8,8];
    }

    $scope.$on('leafletDirectiveMarker.mouseover', function (event, args){
        growMarker(args.modelName);
    });
    $scope.$on('leafletDirectiveMarker.mouseout', function (event, args){
        shrinkMarker(args.modelName);
    });

Since a recent update to angular-leaflet-directive, each time a marker's iconSize and iconAnchor properties are changed, my console log gets filled with a debug log message for each and every marker on the map, regardless of whether that specific marker was changed:

[AngularJS - Leaflet] [marker] is already rendered, marker: king
angular.js:12314 
[AngularJS - Leaflet] [marker] is already rendered, marker: mathsci
angular.js:12314 
[AngularJS - Leaflet] [marker] is already rendered, marker: art
angular.js:12314 
[AngularJS - Leaflet] [marker] is already rendered, marker: pes
angular.js:12314 

...etc. I have about 20 markers, so I get 20 errors when I mouseover and 20 errors when I mouseout.

To fix this for now, I've turned off Angular debug logging in my application's configuration (it is turned on by default):

angular.module(ApplicationConfiguration.applicationModuleName).config(['$logProvider',
    function($logProvider) {
        $logProvider.debugEnabled(false);
    }
]);
@nmccready
Copy link
Contributor

It would be better if angular-leaflet internally used an independent logger which disables everything but error by default. Then users can choose to turn it on.

@MattSidor
Copy link
Author

Thanks @nmccready, that sounds perfect to me.

@Wilzi
Copy link
Contributor

Wilzi commented Aug 29, 2015

+1

@nmccready nmccready self-assigned this Aug 30, 2015
@nmccready nmccready added this to the 0.8.8 milestone Aug 30, 2015
@nmccready
Copy link
Contributor

About 75% of the way there I ported ui-gmap's logger to https://github.com/nmccready/angular-simple-logger so both code bases will be sharing that dependency.

@nmccready
Copy link
Contributor

PR #915

@MattSidor
Copy link
Author

Thank you very much @nmccready

@simison
Copy link
Contributor

simison commented Sep 5, 2015

Could you add this requirement to README.md as well? https://github.com/tombatossals/angular-leaflet-directive#how-to-use-it
This change isn't mentioned at CHANGELOG.md either.

Actually this would've deserved 0.9 (breaking change) since it requires embedding another file?

It's easy to overlook it as seen at #918

Cheers!

@nmccready
Copy link
Contributor

I was on th fence on versioning but why do you need a Readme. The requirements are in both package.json and Bower.json I think that's clear enough.

@simison
Copy link
Contributor

simison commented Sep 5, 2015

As I said, it's easy to overlook this ;-) Expect anything from the people — I'm sure we've both seen the dumbest ever questions floating the internets. If you don't want to spend any time in the future directing people to look at that extra file, you could spend 3mins to mention it at those text files. :-)

On more serious tone;
It's easy to miss something like this when just hitting bower update and you have a ton of packages. I always check change logs and I'm even following this repo, so I tend to notice these things.

My staging server's CI on the other hand does npm+bower update and since this version wasn't supposed to be a breaking change, bower would've just bluntly updated this to 0.8.8. (I use ~0.8.7 requirement for this kind of packages to allow minor updates).

Not a biggie but food for thought for the future.

PS. Thanks a bunch working on this! It's an amazing library and I rely a lot on it at @Trustroots.

@nmccready
Copy link
Contributor

Ok, I'll take a PR with a readme update or a specific markdown for like dependencies.md or DEPENDENCIES.md. It could include advice on how to deal with getting the dependencies into a persons vendor script etc..

Thanks for the kind feedback.

I just hope more people do a little more digging like yourself (@simison); and make an attempt to figure things out on their own. If more people did that then they could give back; instead of everything being a magic black box.

@nmccready
Copy link
Contributor

Should I bump to 0.9.X anyway or keep 0.8.X I can go either way.

@simison
Copy link
Contributor

simison commented Sep 5, 2015

Now that I read http://semver.org/ myself again — it just reminds me how unclear this stuff is with versioning... to bump or not, hmm! Perhaps observe for a while if mention at changelog stops those issues coming in (was that recent one 3rd already today?)

Anyway, I'm proposing this; #922 and #923 — thoughts?

@nmccready
Copy link
Contributor

Looks good, I'm sure we'll get at least 10 more issues of the same thing.

@simison
Copy link
Contributor

simison commented Oct 6, 2015

Looking back now, this should've been 0.9.X. ;-) Lessons learnt.

@nmccready
Copy link
Contributor

Yup

@omeid
Copy link

omeid commented Dec 17, 2015

Creating a new module and adding an external dependency for such a trivial (compared to the scope of project) is rather unjustified. @nmccready Did you consider pulling the logging code into a separate module that is still part of leaflet.js?

@nmccready
Copy link
Contributor

@omeid I'm not on this project anymore but it is justified when it is being used in more than one library. People use project everyday that have dependencies, get used to it. Feel free to remove it as I don't care.

@MattSidor
Copy link
Author

@nmccready your tone is awfully harsh, I think he was just trying to understand your rationale better.

@nmccready
Copy link
Contributor

sorry I don't respond well to unjustified . Anyway I'll never please everyone.

@omeid
Copy link

omeid commented Dec 17, 2015

@nmccready The fact that people leave projects is exactly why I think adding an external dependency that is managed by a single contributor is unjustified and unreasonable.

As, @seadour explained, I asked why you thought it was a better idea to add the dependency instead of creating an internal module so other's don't repeat the same mistake. No need to get emotional.

@nmccready
Copy link
Contributor

Apologies. Anyways I am not leaving the logging project as it is simple and I find it personally very useful. We use it at work heavily and it is in ui-leaflet as well as other projects. Anyways with that said I think @tombatossals removed it from this repo anyways. So there is really is nothing to discuss unless your using ui-leaflet. So honestly I don't know why this is being discussed here. Look at 0.10.X its gone.

Again the rationale of it originally was to not have copy paste code in multiple projects which was useful. I don't really see why it is such an issue. Look at this project it has many deps and those deps have deps...

https://github.com/bendrucker/angular-stripe

I originally solved this whole logging issue in angular-google-maps and wanted the code here to not maintain it in two places.

@omeid
Copy link

omeid commented Jan 12, 2016

@nmccready That project has zero dependency, other than of course, angular.js

Anyways, I originally decided to try angular-uis fork but the extra dependency made me decided against it. Specially when the rationale behind the fork was creating a fork that is managed in a more structured way.

@nmccready
Copy link
Contributor

@omeid I am unsubscribing from this if you want to talk about this more then move to the relevant issue/repo angular-ui/ui-leaflet#210

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