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

Change moment to be a peerDependency and a devDependency #622

Closed
wants to merge 1 commit into from
Closed

Change moment to be a peerDependency and a devDependency #622

wants to merge 1 commit into from

Conversation

iMarv
Copy link

@iMarv iMarv commented Jun 6, 2018

Currently, when using moment-timezone, moment is installed as a dependency.
This means if I have moment installed as a dependency in general, it won't be reused. Instead, moment-timezone uses its own version of moment.

This is leading to issues with a locale magically not being set or similar, as the "wrong" moment dependency is being used.

With this pull-request, moment is defined as a peer-dependency, which means it will be reused from an existing dependency. In a dev setup, we want to install moment in any case, this is why I added it to the devDependencies aswell.

@jsf-clabot
Copy link

jsf-clabot commented Jun 6, 2018

CLA assistant check
All committers have signed the CLA.

@ellenaua
Copy link
Contributor

Thanks, this will be in release 0.5.18

@ellenaua ellenaua closed this Jun 18, 2018
@dominykas
Copy link

dominykas commented Jun 18, 2018

This breaks things very subtly... It's a pity this is not v1 yet, so no way to indicate via semver and there's no tag under releases here (but thanks for the changelog!)

In our case, we never used moment directly and always relied on moment-timezone to bring it in (i.e. if you have moment-timezone - you likely don't need moment in your app?).

This means that since npm no longer breaks on missing peer deps for quite a while, you only get a warning (which is easy to miss) and your app breaks the moment you do require('moment-timezone') unless you have moment actually installed.

I would argue this should be unpublished and the earlier behavior restored and then possibly publish a v1 instead with this change, if it's really desirable?

Anyways, thanks for the good work, and I hope there won't be an entitled angry mob, which sometimes descends on maintainers when things break...

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.

None yet

4 participants