-
Notifications
You must be signed in to change notification settings - Fork 172
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
Update graphql/graphql-relay and add support for Sequelize 6 #715
Conversation
to be honest this is even more difficult. I don't think the unit tests pass in Sequelize 3 which are required in devDeps due to instanceof failures in the typeMapper:
If I install Sequelize 6 for testing locally, then I get Promise errors as it expects bluebird which is no longer there... |
@intellix I'd be fine with dropping the legacy support, can publish as a major. The project needs some maintenance of dependencies, running tests is troublesome yes. |
69dc855
to
11c554e
Compare
11c554e
to
b7215e8
Compare
ok all unit and integration tests now pass with:
And Sequelize v6+ supported for dataloading (due to the constructor.options and findByPk changes) |
Awesome work @intellix, thank you! |
"graphql": "^0.5.0 || ^0.6.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14", | ||
"graphql-relay": "^0.4.2 || ^0.5.0 || ^0.6.0", | ||
"graphql": "^0.5.0 || ^0.6.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14 || ^15 || ^16", | ||
"graphql-relay": "^0.4.2 || ^0.5.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0", | ||
"sequelize": ">=3.0.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.
Does this need to be bumped also?
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.
yep, this is one of the main complaints I get when I try to update packages, because 0.10.0 has a peerDep on 16+ https://github.com/graphql/graphql-relay-js/blob/main/package.json#L43
For 16 you need 0.10 and the lower ones still work as well
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.
@intellix Ah sorry, no I was curious whether we should also bump the peer dep requirement for sequelize
. Would v3 still work with these updates?
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.
yep, apart from the tests now running in v6, all I really added for sequelize was the findByPk in the ternary and fallback to constructor.options
@intellix Sorry about that, didn't realize this repo didn't have a prepublish script, please try |
Was hoping to update to Apollo Server 4 and noticed a plethora of peer dependencies getting angry. Noticed that the peer deps here were quite old. Tried: