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/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
markushi
left a comment
There was a problem hiding this comment.
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. |
lbloder
left a comment
There was a problem hiding this comment.
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>::executeandApolloCall<D>::executeV3(behaving as v3).📝 Checklist
sendDefaultPIIis enabled.