-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Conversation
@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 |
There was a problem hiding this 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?
I'm fine with it as well and agree with @benmccann, we should apply the same rule for |
@jsg2021 could you rebase this and apply the same rule to |
sure, i’ll try to do it tomorrow. |
to prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semvar range)
done :) |
package.json
Outdated
} | ||
} | ||
} |
There was a problem hiding this comment.
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
sorry, my editor does that in save. my first edit was directly from github’s online editor. i’ll put it back. |
Related question: shouldn't we avoid to explicitly upgrade dependency versions to the latest unless we really need to? It seems that I'm referring to this kind of changes which, except for ESLint 4, didn't require any explicit upgrade of other dependencies. Thoughts? |
I only changed the |
It's more a question than something I would like to change in this PR (I'm fine with I'm just wondering if explicitly upgrade dependencies without reason is a good practice or not: let say your app requires |
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. |
I did some reading and it appears that if you say It probably makes sense for us to use the lowest compatible version. The lowest versions our tests pass with are:
For |
i can update to those versions if you like. |
@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. |
Prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semver range)
Get rid of custom moment dependency resolutions as chart.js stopped having a fixed moment version in its dependencies: see chartjs/Chart.js#4891.
Prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semver range)
to prevent duplicate version of moment.js (which is pretty large) when an app also depends on moment (with less strict semvar range)