Relaxes nodent-runtime dependency requirement

smeijer / leaflet-geosearch

A geocoding/address-lookup library supporting various api providers.

https://smeijer.github.io/leaflet-geosearch/

MIT License

1.05k stars 273 forks source link

Relaxes nodent-runtime dependency requirement #115

Closed jlengrand closed 7 years ago

jlengrand commented 7 years ago

Now uses latest instead of a specific version

This is an attempt to avoid issues for people using older versions of npm.

Related to #111

Another alternative woudl be to ask for nodent-runtime version to be >= 2.0.0

smeijer commented 7 years ago

Please ignore travis for the moment.

Quote from the related issue:

Regarding the nodent issue. Could we not change the PR so that it needs a version of nodent-runtime rather than a specific version? That should fix it for everbody, shouldn't it?

You're now setting it to "latest". I'm not sure how node handles this, but isn't the "latest" in this case always required? Does this solve the issue, or is it better to set it as you suggested, just higher than 2x?

The presence of runtime should be fine.

jlengrand commented 7 years ago

Please note that I am far from being a node expert :).

My impression:

latest will always get the latest available. This is less restrictive, but could lead to breaking changes in case for example noent_runtime introduces some breaking changes in the future.
=2.0.0 is more conservative (as in it probably would be nice to update it once in a while), but I would expect it will take the closest from the requirement if you don't have it yet. If the user already has it globally installed, I'd expect it takes the existing.

More info here.

All in all, it depends how bleeding edge you want to be with geosearch and how safe nodent-runtime is in newer versions. I can update the PR with any solution you might prefer :)

smeijer commented 7 years ago

Okay, let's try the 3.x.x range. I must depend on you in this one. As I'm not experiencing errors with this dependency.

I choose nodent-runtime over a babel-polyfill because it keeps the bundle smaller. nodent-runtime translates the await / async code to ES5. I think we need to switch to a babel-polyfill later, if we keep getting issues with this one.

Can you rebase on develop? I've just pushed one very small update (spacing issue), causing travis to fail.

smeijer commented 7 years ago

I've changed the base branch. I think rebasing is no longer needed 😄

jlengrand commented 7 years ago

alexrussell commented 7 years ago

FWIW I doubt this will have any effect on me - I am installing leaflet-geosearch as a dependency so I won't get its devDependencies in my project (as they are only used to build it).

smeijer commented 7 years ago

@alexrussell I think you're right. I'm moving the dependency from devDependencies to dependencies instead. That should fix your problem.

alexrussell commented 7 years ago

@smeijer it's worth having a quick read of my latest comment on #111 though first. Pushing nodent-runtime on your users may not be ideal.

smeijer commented 7 years ago

True, lets continue the discussion in #111, to prevent cross-talk.

smeijer commented 7 years ago

Thanks for your work on this one. As we have implemented a working solution in #111 and #112, i'm closing this one.