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

switch to peerDependencies #50

Merged
merged 1 commit into from
Feb 24, 2022
Merged

switch to peerDependencies #50

merged 1 commit into from
Feb 24, 2022

Conversation

gavinr
Copy link
Contributor

@gavinr gavinr commented Feb 24, 2022

Inspired by jgravois/esri-leaflet-gp#46 (comment), switched leaflet, esri-leaflet, and leaflet.markercluster to be peer dependencies

(more info in previous discussion)

@jgravois if you have time could you please check this out - does it look correct?

switched leaflet, esri-leaflet, and leaflet.marktercluster to be peer dependencies
Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @gavinr 👋

i definitely still think this is a viable way to eject and leave it up to consumers to pick whatever they want. if it helps, no one has complained since i made the same change in esri-leaflet-gp.

because of the new ambiguity, you might consider adding a little more information to the CHANGELOG or creating a matrix in the README to explain what version ranges of this plugin correspond with what ranges of esri-leaflet and leaflet.

if i had it all to do over again i'd probably skip sharing rollup configs between esri-leaflet plugins, but such is life. 😄

ps. nothing wrong with checking in a v2 package-lock.json file, but if you're going to, you might want to run the travis tests on Node 16.x too.

Copy link
Contributor

@jwasilgeo jwasilgeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, too, @gavinr. I also 2nd the pearls of wisdom @jgravois already dropped in this PR discussion.

@gavinr
Copy link
Contributor Author

gavinr commented Feb 24, 2022

Thanks @jgravois!

might consider adding a little more information to the CHANGELOG

So would this just be a note in the changelog like This release of esri-leaflet-cluster was tested on [email protected] and [email protected]. Newer minor versions of those libraries will probably work, but be sure to test before upgrading.?

run the travis tests on Node 16.x too

Great point. Apparently the travis.org tests stopped working back in june 2021: https://travis-ci.org/github/Esri/esri-leaflet-cluster. I think we need to switch over to GitHub Actions or setup a connection to the new travis-ci.com for all esri-leaflet-* repos. I created an issue: Esri/esri-leaflet#1318.

@gavinr gavinr merged commit 8343c28 into Esri:master Feb 24, 2022
@gavinr gavinr deleted the peer-dependencies branch February 24, 2022 20:34
@jgravois
Copy link
Contributor

Newer minor versions of those libraries will probably work

I was thinking more about trying to tease out the minimum version of Leaflet/Esri Leaflet for each major release here.

esri-leaflet-cluster esri-leaflet leaflet
v1.x.x ? ?
v2.x.x ? ?

@gavinr
Copy link
Contributor Author

gavinr commented Feb 25, 2022

This has been released in v3.0.0 https://github.com/Esri/esri-leaflet-cluster/releases/tag/v3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants