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

Move moment to peerDependencies #1168

Merged
merged 1 commit into from Dec 17, 2017

Conversation

Kerumen
Copy link
Contributor

@Kerumen Kerumen commented Dec 11, 2017

Supersedes #419

Fixes #122

It's better to move moment to peerDependencies because it prevents duplication of moment instances which can leads to bugs (when loading locals for instance).
In our project, we have a fixed dependency of moment@2.18. react-datepicker installs moment@2.19 so 2 instances of moment are loaded and locals doesn't work for the moment of react-datepicker.

If you look at react-dates, this is how they require moment: https://github.com/airbnb/react-dates/blob/master/package.json#L126

I also removed package-lock.json as it's not used in the project anymore. Tell me if I should remove this from the PR as it's not quite related (but I didn't know which lockfile to update so I thought it should be better to delete it).

@martijnrusschen
Copy link
Member

I agree this is probably better, however there's some discussion going on here #122.

@martijnrusschen
Copy link
Member

Not sure if we can make this change without breaking everyone's datepickers. What do you think?

@codecov
Copy link

codecov bot commented Dec 11, 2017

Codecov Report

Merging #1168 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1168   +/-   ##
=======================================
  Coverage   85.36%   85.36%           
=======================================
  Files          13       13           
  Lines         888      888           
  Branches      146      146           
=======================================
  Hits          758      758           
  Misses         42       42           
  Partials       88       88

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d513e83...b6bea21. Read the comment docs.

@Kerumen
Copy link
Contributor Author

Kerumen commented Dec 11, 2017

Some discussion which ended 1 year and a half ago. I think this should be reconsidered.

I think that everyone who uses the datepicker are already using moment but if we want to be truely semver compliant, this can be released as a breaking change (major).

@aij
Copy link
Contributor

aij commented Dec 11, 2017

@Kerumen I think the plan is to bump the major only when we think the API is stable. Keeping it at 0 is also truly semver compliant since semver says

  1. Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

@Kerumen
Copy link
Contributor Author

Kerumen commented Dec 11, 2017

@aij Yes, seems legit too. npm won't auto-upgrade people and if they manually do it, they will know why their code breaks (if it breaks).

@martijnrusschen
Copy link
Member

Okay lets do this. We use yarn.lock instead of the package-lock.json, so lets make sure that's updated.

@martijnrusschen martijnrusschen added this to To Do in Pull requests Dec 16, 2017
@martijnrusschen martijnrusschen moved this from To Do to Waiting on merge conflict in Pull requests Dec 16, 2017
@martijnrusschen
Copy link
Member

@Kerumen Let me know if you want to proceed with this, happy to merge now we're reaching consensus.

@Kerumen
Copy link
Contributor Author

Kerumen commented Dec 17, 2017

@martijnrusschen Conflicts resolved. I set ^2.0.0 as semver hence people won't have warnings if they install a previous version than 2.19.4. I suppose all moment >=2 works.

@martijnrusschen martijnrusschen merged commit 9c3bf40 into Hacker0x01:master Dec 17, 2017
Pull requests automation moved this from Waiting on merge conflict to Done Dec 17, 2017
Squar89 pushed a commit to Squar89/react-datepicker2 that referenced this pull request Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Pull requests
  
Done
Development

Successfully merging this pull request may close these issues.

Moment.js as a peer dependency
3 participants