-
Notifications
You must be signed in to change notification settings - Fork 557
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 CAPI & FAPI clients to latest versions (also remove hack required for Lightbox) #25139
Conversation
d7cd765
to
8e97136
Compare
Deployment to CODE succeeded almost everywhere except for facia-press. We should release a beta version of facia-client: https://logs.gutools.co.uk/s/dotcom/goto/02ce2d00-f24e-11ec-9ed0-71515ea686c5 |
We compared and they were the same: |
I've rolled back the Frontend CODE environment to On our branch, without updating
This is the code in Future.traverse(searchQueries)(client.getResponse(_)) |
We're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
We're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
1ec73d4
to
9864128
Compare
We're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
9864128
to
0980d5f
Compare
We're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
8c43ce2
to
71b241d
Compare
We're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
71b241d
to
fd9fba0
Compare
Deploying build 43804 (71b241d - which has the FAPI client updated to use the same CAPI client with guardian/facia-scala-client#271) to CODE has completed - happily, there are no further The lightbox functionality from #20462 is also working: Screen.Recording.2022-06-30.at.11.48.37.mov |
The latest version of the CAPI client (v19) includes guardian/content-api-scala-client#359, which we can use to replace the pagination code based on `play-iteratees` (https://github.com/playframework/play-iteratees - introduced in July 2014 with #5199), currently blocking the Scala 2.13 upgrade for Frontend (see #24817). The updated CAPI client is binary-incompatible with previous versions of the CAPI client, meaning we also need to update the FAPI client as it also uses CAPI. The updates to the CAPI client also support `previous`/`next` search functionality, as used by the Lightbox, meaning that the small `ContentApiNavQuery` hack for the Lightbox (introduced here: #20462) can now be removed. A similar PR updating FAPI & CAPI in Ophan is at guardian/ophan#4719. Co-authored-by: Roberto Tyley <[email protected]> We're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
fd9fba0
to
1081f7e
Compare
The latest version of the CAPI client (v19) includes guardian/content-api-scala-client#359, which we can use to replace the pagination code based on `play-iteratees` (https://github.com/playframework/play-iteratees - introduced in July 2014 with #5199), currently blocking the Scala 2.13 upgrade for Frontend (see #24817). The updated CAPI client is binary-incompatible with previous versions of the CAPI client, meaning we also need to update the FAPI client as it also uses CAPI. A similar PR updating FAPI & CAPI in Ophan is at guardian/ophan#4719. Like there, we're adding a test that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible. The updates to the CAPI client also support `previous`/`next` search functionality, as used by the Lightbox, meaning that the small `ContentApiNavQuery` hack for the Lightbox (introduced here: #20462) can now be removed. See also: * #25139 (comment) * https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529 Co-authored-by: Roberto Tyley <[email protected]>
1081f7e
to
0025248
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.
💯
Seen on PROD (merged by @ioannakok 18 minutes and 23 seconds ago)
|
Deployed on PROD, and the lightbox still works! Screen.Recording.2022-07-01.at.16.39.39.mov |
This test, introduced with #25139, is intended to fail if incompatible versions of CAPI & FAPI are used. However, it also failed when we switched to Scala 2.13 - there seems to be some difference in the way Future.traverse behaves between the two versions of Scala - the `null` values returned by the Mockito mock didn't cause a problem in Scala 2.12, but threw a `NullPointerException` in Scala 2.13 (in `Future.zipwith`). Easiest fix is to use a stub, that returns non-null objects, rather than a mock. The requests made by this stub ContentApiClient will never succeed, but we've verified that the calls do indeed only fail with an exception when the versions of CAPI & FAPI are incompatible.
What does this change?
The latest version of the CAPI client (v19) includes guardian/content-api-scala-client#359, which we can use to replace the pagination code based on
play-iteratees
(introduced in July 2014 with #5199), currently blocking the Scala 2.13 upgrade for Frontend.The updated CAPI client is binary-incompatible with previous versions of the CAPI client, meaning we also need to update the FAPI client - as it also uses CAPI, and needs to be compiled against the same CAPI version. A similar PR, updating FAPI & CAPI in Ophan is at https://github.com/guardian/ophan/pull/4719, and has been successfully deployed. Like there, we're adding a
FaciaClientTest
that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible.The updates to the CAPI client also support
previous
/next
search functionality, as used by the Lightbox, meaning that the smallContentApiNavQuery
hack for the Lightbox (introduced here: #20462) can now be removed.Screenshots
The next/previous functionality in lightboxes (eg on this article) from #20462 continues to work:
Screen.Recording.2022-06-30.at.11.48.37.mov
Checklist
Does this change update the version of CAPI we're using?
Tested