-
-
Notifications
You must be signed in to change notification settings - Fork 448
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
Add GraphQL Apollo Kotlin 4 integration #4166
Conversation
NOTE: to review this I would recommend just looking at the diff between the first commit in the PR and the last one, the first commit is porting the code we have for v3 to v4 |
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c2c78de | 415.28 ms | 505.08 ms | 89.80 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c2c78de | 1.58 MiB | 2.21 MiB | 640.27 KiB |
Previous results on branch: lcian/feat/apollo-4
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b6f49c8 | 347.02 ms | 419.10 ms | 72.08 ms |
afa0712 | 379.70 ms | 463.00 ms | 83.30 ms |
98d90f9 | 410.86 ms | 515.15 ms | 104.29 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b6f49c8 | 1.58 MiB | 2.21 MiB | 641.11 KiB |
afa0712 | 1.58 MiB | 2.21 MiB | 641.06 KiB |
98d90f9 | 1.58 MiB | 2.21 MiB | 641.09 KiB |
sentry-apollo-4/src/test/java/io/sentry/apollo4/generated/type/GraphQLBoolean.kt
Show resolved
Hide resolved
...pollo-4/src/test/java/io/sentry/apollo4/generated/selections/LaunchDetailsQuerySelections.kt
Show resolved
Hide resolved
sentry-apollo-4/src/main/java/io/sentry/apollo4/SentryApollo4.kt
Outdated
Show resolved
Hide resolved
sentry-apollo-4/src/main/java/io/sentry/apollo4/SentryApollo4ClientException.kt
Show resolved
Hide resolved
...y-apollo-4/src/test/java/io/sentry/apollo4/SentryApollo4BuilderExtensionsClientErrorsTest.kt
Show resolved
Hide resolved
sentry-apollo-4/src/test/java/io/sentry/apollo4/SentryApollo4HttpInterceptorTest.kt
Show resolved
Hide resolved
…nsions to make it easier to migrate
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.
NOTE: to review this I would recommend just looking at the diff between the first commit in the PR and the last one, the first commit is porting the code we have for v3 to v4
i.e. use this to review: 4fac45da..59977875 (#4166)
I mainly reviewed exactly that, looks good to me! Do you think we should release this in an alpha version first?
Thanks @markushi ! |
Since this adds a new module and doesn't change existing code I think it's safe to release without an alpha as the only customers affected are those explicitly using the new Apollo 4 dependency and actively configuring it to be used. |
Deferring to @lbloder for a review here since he wrote the integration for v3. |
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, nicely done 👍
To make the CI run successfully, you'll need to run make api
once locally and push the changes.
Thanks for kickstarting this @cvb941 ! |
thanks @lcian for the work here, I hope many people will use the updated Apollo integration as well 😊 |
📜 Description
Adds a new module to integrate Apollo Kotlin 4.
💡 Motivation and Context
Closes #3662
💚 How did you test it?
Unit tests with both
ApolloCall<D>::execute
andApolloCall<D>::executeV3
(behaving as v3).📝 Checklist
sendDefaultPII
is enabled.