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

Relax momentjs semvar range #4891

Merged
merged 4 commits into from
Nov 7, 2017
Merged

Relax momentjs semvar range #4891

merged 4 commits into from
Nov 7, 2017

Conversation

jsg2021
Copy link
Contributor

@jsg2021 jsg2021 commented Oct 25, 2017

to prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semvar range)

@benmccann
Copy link
Contributor

@simonbrunel @etimberg this seems like a reasonable change to me. what do you think? should we also make the same change to the usage of chartjs-color if we go with this PR?

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I'm ok with relaxing the range though it may have test implications. Is it possible to run our tests with different moment versions automatically?

@simonbrunel
Copy link
Member

I'm fine with it as well and agree with @benmccann, we should apply the same rule for chartjs-color for consistency.

@benmccann
Copy link
Contributor

@jsg2021 could you rebase this and apply the same rule to chartjs-color?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 4, 2017

sure, i’ll try to do it tomorrow.

jsg2021 and others added 2 commits November 4, 2017 19:42
to prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semvar range)
@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 5, 2017

done :)

package.json Outdated
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i'm not sure we should remove the last line

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 5, 2017

sorry, my editor does that in save. my first edit was directly from github’s online editor. i’ll put it back.

@simonbrunel
Copy link
Member

Related question: shouldn't we avoid to explicitly upgrade dependency versions to the latest unless we really need to? It seems that "moment": "^2.19.0" is better than "moment": "^2.19.1" (^2.0.0 could also work unless we need a feature introduced in ^2.19.0).

I'm referring to this kind of changes which, except for ESLint 4, didn't require any explicit upgrade of other dependencies.

Thoughts?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 6, 2017

I only changed the ~ to ^...when I rebased I picked up the new number... I can change it to 2.18.0 to match the last release

@simonbrunel
Copy link
Member

It's more a question than something I would like to change in this PR (I'm fine with ^2.19.1).

I'm just wondering if explicitly upgrade dependencies without reason is a good practice or not: let say your app requires ~2.18.0 and this lib depends on ^2.19.1, I guess that the dependency will be duplicated (2.18.0 and latest 2.x.x). However, what happen if Chart.js depends on ^2.0.0? does this prevent duplication of the dependency?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 6, 2017

Ah, yes. that would prevent duplication... if this project does not explicitly depend on something in 2.19.1, it should be set as low as possible to allow for that scenario.

@benmccann
Copy link
Contributor

benmccann commented Nov 6, 2017

I did some reading and it appears that if you say ^2.18.1 then that means version >= 2.18.1 && version < 3.0.0

It probably makes sense for us to use the lowest compatible version. The lowest versions our tests pass with are:

"chartjs-color": "^2.1.0",
"moment": "^2.10.2"

For devDependencies I'd probably lean towards using the highest version of whatever is available to keep developer environments as consistent as possible

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 6, 2017

i can update to those versions if you like.

@simonbrunel
Copy link
Member

@jsg2021 can you update the PR versions please for the one suggested by @benmccann? Then let's stop upgrading dependencies (including devs) as long as it's not required.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 7, 2017
@simonbrunel simonbrunel merged commit 67479c2 into chartjs:master Nov 7, 2017
@jsg2021 jsg2021 deleted the patch-1 branch November 7, 2017 19:56
@simonbrunel simonbrunel mentioned this pull request Nov 16, 2017
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
Prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semver range)
tusbar added a commit to etalab/geo.data.gouv.fr that referenced this pull request Mar 22, 2018
Get rid of custom moment dependency resolutions as chart.js stopped
having a fixed moment version in its dependencies: see
chartjs/Chart.js#4891.
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semver range)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants