Change dependencies to peerDependencies

[semeleven]

If I try required Proj4Leaflet and have local version leaflet, then I have two version - your and my.

[semone]

Nice of you to contribute! According to Leaflet plugin guide you should define it as a dependency.

Also see the comment on this PR #117 on why it might be so.

Maybe having it as a devdependency would be better than peerdependency?

[semeleven]

Done, in leaflet plugin guide now peerDependencies

[semone]

Hmm I still think having it in the peerDependencies is wrong, and it should be in the devDependencies if anything.

I also found this issue which kind of says don't use peerDependencies since it is removed from all official leaflet plugins.

[perliedman]

My current view is that it should be a peerDependency or not a dependency at all (you will notice when you try to use the plugin without Leaflet).

Having it as an explicit dependency causes the problems @Ralf8686 mentions, and it's particularly bad if you try to use plugins written for 0.7 with Leaflet 1.0.

We should still have Proj4 as a dependency, though. Peer dependencies are typically used when you write a plugin, like Proj4Leaflet is a plugin to Leaflet, but Proj4Leaflet consumes Proj4, it isn't a plugin for Proj4.

[semone]

Okey maybe I should admit myself defeated! But I still don't see the difference between an official plugin and this plugin?

[perliedman]

@semone hmm, maybe I wrote it in a confusing way.

What I meant to say was that @Ralf8686's patch turns both Leaflet and Proj4 (note: Proj4.js, not Proj4Lealfet itself) into peer dependencies. I think Leaflet should be a peer dependency, or not a dependency at all, but Proj4 should remain a dependency.

[semeleven]

@perliedman I don't sure about Proj4 too. Because , example, in my project i use Proj4 too. I think, all plugins should use peer dependencies, to not have conflicts with local dependencies. Or am I wrong?

[perliedman]

@Ralf8686 for most libs, having two instances of it is no problem, npm and CommonJS is built on that prinicple. It becomes a problem with libs like Leaflet, that use a window global. I don't see the same situation with Proj4.

[theashyster]

I am confused now - so this was a wrong thing to do? Leaflet/Leaflet#4534

[semone]

That is what confuses me too @theashyster

[perliedman]

I'm not sure when or why the recommendation in Leaflet's plugin guide changed to peer deps. I'm guessing since normal deps cause problems for plugins that would otherwise work with both Leaflet 0.7 and 1.0.

But if you use Browserify or similar and need a plugin to be compatible with multiple versions of Leaflet, standard dependencies will make things break.

As mentioned above, I'm fine with just dropping the dependency as well, but you could argue that a peer dependency is nice since it's a machine readable recommendation.

[semone]

The reason the plugin guide changed to peer is because @Ralf8686 made a PR to the plugin guide for it to change today. Leaflet/Leaflet#5142

[theashyster]

@semone @perliedman Looks like there is not a clear solid position of what is the correct way of doing this.

The problem here is that before peerDependencies caused issues for me when they were set as that in Leaflet.Editable, so this was created Leaflet/Leaflet#4534 by @hyperknot after a debate between us and @yohanboniface and right now it looks like Leaflet.Editable uses Leaflet as devDependencies and probably Proj4Leaflet should do that too?

I see that having it as devDependencies does not allow this plugin to be just installed automatically and everything to work out of box, but peerDependencies does not do that either, it would just show a warning.

I agree that standard dependencies can make things break and Leaflet probably should not be a part of them for plugins, I think they should be completely removed or added to devDependencies, because who would use a Leaflet plugin without Leaflet itself?

Yeah, so @Ralf8686 what do you say about it being inside devDependencies?

[semone]

I fully agree with you @theashyster that is how I have understood it as well.

[pthorin]

So, is consensus that we should list Leaflet inside devDependencies?

[theashyster]

@pthorin I still think that Leaflet should go in devDependencies, but that is just me.

[perliedman]

I think the situation with Leaflet.Editable is a bit different: they actually build the library, for which I guess they need Leaflet to be installed. Proj4Leaflet has no build step, so Leaflet itself is needed as much for dev as it is for just using it.

My view is that putting it in devDependencies is just weird, it doesn't help anyone. The warning we get from peerDependencies could actually help someone, but I can also understand the issues some people have with it, so I'm also fine with just not having a Leaflet dependency at all in package.json.

[theashyster]

Okay, I see, good point.

Okay, I just thought that putting it in peerDependencies will cause trouble again, but maybe it won't. I agree that the warning could help, but for us the build actually failed because of the warning / error, that is why I am kind of scared of it. In the ideal world where every library that we use uses a correct version of Leaflet and the other dependencies, there should be no such warnings, I guess.

So I say we can try to go both the peerDependencies and the remove Leaflet dependency ways, but I like the 2nd one better then. :)

[pthorin]

Ok, will remove Leaflet from dependencies. This also makes it easier for people to manage when Leaflet updates to say 1.0.4 etc.