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

update to 0.9.0 fails for debug dependency, bower when bundled via browserify #997

Closed
nschloe opened this issue Oct 16, 2015 · 16 comments
Closed

Comments

@nschloe
Copy link

nschloe commented Oct 16, 2015

At the last update of leaflet (to 0.8), we've had problems since the new simple-logger dependency wasn't available in bower. We're going through the same thing for 0.9 because apparently simple-logger now depends on debug which isn't available for bower.

@nmccready
Copy link
Contributor

Which js file are you pointing to for the logger? You should be using angular-simple-logger.js or angular-simple-logger.light.js as they don't require debug as it is wrapped or not included. Otherwise use npm and use browserify/webpack.

@nschloe
Copy link
Author

nschloe commented Oct 16, 2015

We're just using the leaflet directive which depends on logger which now depends on debug. We're managing all of that through bower/browserify, but like I said, it's become somewhat painful to maintain the dependencies.

@nmccready
Copy link
Contributor

So if your using browserify, don't use bower for this library. Use the npm version. The visionmedia debug, part is what makes this an issue as its browser version will break browserify. Just as the bower version of angular-simple-logger.js is made with browserify already and will break commonjs if used. So use the commonJS version of index.js. The dependencies are propagated correctly on npm.

Visionmedia's debug has issues in bower where only using the browser version or browserifying it for bower (angular-simple-logger) will work. This is becuase they decided to not include any of their dependencies which are on npm. https://github.com/visionmedia/debug/blob/2.2.0/bower.json

From their point of view using the https://github.com/visionmedia/debug/blob/2.2.0/browser.js is sufficient. Which I am essentially doing the same by browserifying angular-simple-logger.js for bower.

I don't really know any other way around this sorry. But npm is your better choice or use the light version with no debug.

@nmccready
Copy link
Contributor

To discuss this more please file an issue on angular-simple-logger.

@nmccready
Copy link
Contributor

The other problem is I don't have perms yet to push angular-leaflet-directive to npm. If I did then you could get this project from there and avoid this problem.

I am adding some comments about this to the Readme.md on angular-simple-logger.

@nmccready
Copy link
Contributor

#989 @tombatossals 💥

@nmccready
Copy link
Contributor

@nmccready
Copy link
Contributor

One possible improvement to avoid your personal h3ll, is to make the *light.js version the bower default. But be aware you will not have debug capabilities.

@nmccready
Copy link
Contributor

@nschloe does any of this make sense?

@nschloe
Copy link
Author

nschloe commented Oct 16, 2015

@nmccready , thanks for sharing your insight.

So if your using browserify, don't use bower for this library. Use the npm version.

The existence of a bower.json for the leaflet directive had lured us into thinking that this use case was actually supported. Since we're not using npm for frontend shipping dependencies and manually maintaining all dependencies of leaflet is no acceptable for us, we'll just remove the leaflet dependency altogether from our app.

As a developer, I don't understand why a directive has to ship it's own logging library, especially since, eventually, this is the reason why we can't use the directive. Having a logging tool on the dev box for debugging purposes: alright. But baked into the product? The way it got into leaflet without any review also strikes me as somewhat dubious and raises concerns on how much headache continuing to use this directive would cause us in the future.

Well, anyhow, he who codes is right, so I won't suggest to change anything.

@nmccready
Copy link
Contributor

So you can use bower, but if your dead set in browserifying every library under the sun from bower say for angular-leaflet-directive. Then your going to run into the problems which you are. So what I am saying to do is split your bower up. Use some as browserify and some as a seperate vendor.js file.

So again if you must browserify all your dependencies (AND want the full logger version) then I suggest using npm. This project plans to support both bower and npm. npm being a little rusty right now (delayed releases).

Again if your deadset on browserifying and expect it to not have errors then direct your logger dependency to the light version.

Ultimately I think the less headache solution will be pointing bower to use the light version. That way it will not error out on ng-leaflet. Then a user can decide if they want the advanced debugging features and use bower, browserify, npm to do its magic.

@nmccready
Copy link
Contributor

I made a ticket; I'll try to get to it by Monday and make this for 0.9.1 here. nmccready/angular-simple-logger#12

@nmccready
Copy link
Contributor

Re-opening for 0.9.1 so we point to angular-simple-logger#>=0.1.5

@nmccready nmccready reopened this Oct 16, 2015
@nmccready nmccready changed the title update to 0.9.0 fails for debug dependency, bower update to 0.9.0 fails for debug dependency, bower when bundled via browserify Oct 16, 2015
@nmccready nmccready added this to the 0.9.1 milestone Oct 16, 2015
@nmccready
Copy link
Contributor

As a developer, I don't understand why a directive has to ship it's own logging library, especially since, eventually, this is the reason why we can't use the directive

THIS IS WHY #829 .. I was doing this same code in several projects. Why copy and paste the code?

@nmccready
Copy link
Contributor

This issue was moved to angular-ui/ui-leaflet#133

@nmccready nmccready reopened this Nov 2, 2015
@tombatossals
Copy link
Owner

I'm going to rework&redesign angular-leaflet-directive to be compatible with Leaflet v1.0. It will mantain almost all its functionality, and will be compatible with the current features of the directive, but I must start from a fresh point, so I'm going to close this issue. If you think it must be worked with the new version, please reopen it.

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

3 participants