-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Upgrade dependencies #25
Conversation
61df366
to
3a6bb30
Compare
3a6bb30
to
e8d3257
Compare
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.
Looks good; I didn't know you could do that with the linting - neat! The changes to peerDependencies
need to be reverted (graphile-build/-pg) or modified (pg-sql2), but other than that this all looks good 👍
package.json
Outdated
"graphql": ">=0.6 <15", | ||
"pg-sql2": "^2.2.1" | ||
"pg-sql2": "^4.4.3" |
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.
The change to graphile-build
and graphile-build-pg
is a breaking change as it says to use the postgis plugin you must be running the latest PostGraphile (where that wasn't previously the case). It's safe to update the yarn.lock
(which says which version we're actually using) and the devDependencies
but peerDependencies
and dependencies
must be treated more carefully. For pg-sql2
we want to explicitly state that we support the full range, so:
"pg-sql2": "^4.4.3" | |
"pg-sql2": ">=2.2.1 <5" |
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.
(For graphile-build and graphile-build-pg we already state we support 4.4.4 since ^4.3.0 includes 4.4.0, so we can just change those back to the old values.)
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.
Fixed!
"debug": "^4.1.1", | ||
"tslib": "^1.9.3" | ||
"tslib": "^1.10.0" |
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.
Despite what I said above, these changes to dependencies
are fine 👍
IMO the graphile peerDeps should be removed; they cause a bunch of install-time warnings because users generally won't (and shouldn't be encouraged to) have them installed as direct dependencies in their projects. Instead we should use the |
@mattbretl Personally, I have no particular preference about your point, but it seems unrelated to this pull request, which is about upgrading dependencies, not removing them. Maybe you could make a separate pull request for your change? |
IIRC they're there for TypeScript since we import types from them. If it weren't for that I'd banish them without a second thought. But yes; separate PR 👍 |
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.
LGTM - thanks!
Upgrade all the things