search

onefinestay / react-daterange-picker

Other
563 stars 209 forks source link

Upgrade to moment-range v3, peerDependency #177

Closed wmertens closed 6 years ago

wmertens commented 7 years ago

Needs to be peer dependency to avoid issues with multiple versions of moment and moment-range.

Closes #175

AlanFoster commented 6 years ago

Thanks for the PR @wmertens ! :+1:

What would the impact for an average user be? i.e. Would this require a new 2.x.x release, or would this be backwards compatible for the most part? 🤔

wmertens commented 6 years ago

If they use moment-range themselves, it's a breaking change since moment-range v3 is breaking… if not, makes no difference to them (apart from presumably more robust ranges).

AlanFoster commented 6 years ago

Just noticed there's a failing test now on Travis from this dependency upgrade :)

Before I merge this, I'll have to see if this API bump will work with a codebase that is still making use of the v2 API, otherwise it will have to be a major 2.x.x release of react-dateranger-picker.

For context to my reasoning; The current date picker can be provided with a moment range object directly, as we can see on the examples page - so this might require additional effort for anyone that is using ranges directly with this component 🤔

wmertens commented 6 years ago

@AlanFoster did you see https://github.com/onefinestay/react-daterange-picker/pull/177#discussion_r162862856?

I think it has to be a major update, because you can't mix two versions of moment-range in the same app I would think. Its API changed a bit, something with iterating over dateranges IIRC.

AlanFoster commented 6 years ago

Thanks for the PR! I'll get this merged, try to fix the failing test and publish a major release :+1:

AlanFoster commented 6 years ago

@wmertens I've released this on npm as 2.0.0-rc.1 now, let me know if you have any issues with it. I'll be releasing it as 2.0.0 next week hopefully.

alexfornuto commented 3 years ago

As someone who just discovered this repo today and hit this issue while following the README, may I suggest that you update the README to list this dependency, and anything else that a new user needs to know to install?

P.S. I'm a technical writer, and if you'd care to opt in this repo for Hacktoberfest, I'd be happy to take a pass at updating this and any other stale documentation I come across.