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

Update GraphQL to v14 #2019

Merged
merged 5 commits into from
Jun 3, 2019
Merged

Update GraphQL to v14 #2019

merged 5 commits into from
Jun 3, 2019

Conversation

eloyekunle
Copy link
Contributor

@eloyekunle eloyekunle commented May 23, 2019

@eloyekunle
Copy link
Contributor Author

@znarf

Here's why the Dates will always be coerced to their Unix time representation: https://github.com/graphql/graphql-js/blob/d4140766e0d2add0f82b22fe6e6354de4afc0f3c/src/type/scalars.js#L125.

I'll be using a custom GraphQL scalar type to represent the datetimes.

@eloyekunle eloyekunle changed the title WIP: Update GraphQL to v14 Update GraphQL to v14 May 23, 2019
@eloyekunle eloyekunle requested a review from znarf May 23, 2019 15:30
@coveralls
Copy link

coveralls commented May 23, 2019

Coverage Status

Coverage increased (+0.02%) to 65.012% when pulling 354a14a on feat/update-graphql into e209490 on master.

@znarf
Copy link
Member

znarf commented May 23, 2019

@eloyekunle Looks like a great solution! Do you understand why the test-bdd test are not passing?

@eloyekunle
Copy link
Contributor Author

It's related to expenses. I'm looking into it now.

@eloyekunle
Copy link
Contributor Author

@znarf It's fixed now.

@znarf
Copy link
Member

znarf commented May 23, 2019

@eloyekunle seems like a warning of a breaking change in the API.

What is change in the GraphQL package that is triggering that? Can that be fixed in the API instead of updating the test?

@znarf
Copy link
Member

znarf commented May 25, 2019

I pushed a matching branch on frontend to test if the update is breaking anything in e2e tests.

https://circleci.com/gh/opencollective/opencollective-frontend/tree/feat%2Fupdate-graphql

@eloyekunle
Copy link
Contributor Author

eloyekunle commented May 25, 2019

The breaking change was made here: graphql/graphql-js#1382.

It means cases where the frontend sends Int values as String need to be changed.

@znarf
Copy link
Member

znarf commented May 28, 2019

@eloyekunle Can the breaking change fixed in the API instead of updating the test?

@eloyekunle
Copy link
Contributor Author

eloyekunle commented May 28, 2019

@znarf Trying to fix it means creating a new scalar type for Integers that will also accept Strings and coerce them to Integers.

I've recognized two instances affected in the frontend and opened a PR for it: opencollective/opencollective-frontend#1846.

@znarf
Copy link
Member

znarf commented May 28, 2019

@eloyekunle Thank you for your answer. Hope our coverage is great! 🤞

@eloyekunle
Copy link
Contributor Author

The main things to look out for are cases where the Frontend is currently sending Integers as Strings. In such cases, the String should be parsed to Integer first, before sending to the API.

I'm sure there'll only be a few (if any) such cases left.

@znarf
Copy link
Member

znarf commented May 28, 2019

@eloyekunle What's the behavior if we don't? It's triggering a GraphQL error in the new version?

@eloyekunle
Copy link
Contributor Author

eloyekunle commented May 28, 2019

Yes, it triggers an error. Something like:

GraphQL v1 error: Variable "$id" got invalid value "10890"; Expected type Int; Int cannot represent non-integer value: "10890"

@eloyekunle
Copy link
Contributor Author

Ping @znarf, What's left to merge this?

@znarf
Copy link
Member

znarf commented Jun 3, 2019

@eloyekunle It's good. I usually prefer to merge and deploy changes like that early in the week.

@znarf znarf merged commit b50043a into master Jun 3, 2019
@eloyekunle eloyekunle deleted the feat/update-graphql branch June 3, 2019 11:10
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.

3 participants