CPP-1103 Change x-teaser's props to match content-pipeline's Content #733
Conversation
Now that we are using some nested objects in x-teaser with the introduction of the `teaserMetadata` field, the defensive property accesses we were employing previously have got quite verbose. Let's make use of the more modern optional chaining syntax – which babel supports – to simplify this logic.
| return 'headshot' | ||
| } | ||
|
|
||
| if (props.showImage && props.image && props.image.url) { | ||
| if (props.showImage && props.mainImage?.url) { |
There was a problem hiding this comment.
How can this change support the legacy teaser type? Do we need to change the property name from image to mainImage after fetching content data from esClient in the consumer apps?
I also have a concern about the naming mainImage. It exists in esClient response and it's not equal to the teaser image.
For example, https://www.ft.com/content/2a801534-f3a5-4d0c-8b4f-a790ffa4d702
The mainImage in the es response is
"mainImage": {
"url": "https://next-video-editor-images.s3.ap-northeast-1.amazonaws.com/d0ae9715-f88b-4cb8-98a8-a6d3e361d514",
"width": 1400,
"height": 1400,
"ratio": 1,
"aspectRatio": 1
},
however the teaser image is different
"teaser": {
"image": {
"url": "https://d1e00ek4ebabms.cloudfront.net/production/uploaded-files/ft-news-briefing-2e004171-2412-45a7-969b-31ddb3890ac8.png",
"width": 2048,
"height": 1152
}
}
There was a problem hiding this comment.
How can this change support the legacy teaser type? Do we need to change the property name from image to mainImage after fetching content data from esClient in the consumer apps?
This will be a breaking change so yeah clients using the ElasticSearch API will have to update the field name if they want to migrate to the new major version of x-teaser. I have a branch to do this in next-article as we're in the middle of migrating it to content-pipeline whilst still supporting content-es and the package page uses the x-teaser component directly rather than via cp-content-pipeline-ui. However, there's no rush for other clients using ElasticSearch to update to this new major version of x-teaser, so this change needn't affect them.
I also have a concern about the naming mainImage. It exists in esClient response and it's not equal to the teaser image.
Ah I didn't realise that the teaser image could be different, great point! I'm going to investigate now but we might have to represent this teaser image as teaserMetadata.image instead of mainImage then...
There was a problem hiding this comment.
After the breaking change is released, if we found bugs or x-dash components related to x-teaser need some changes, what are we going to do? Releasing a fix version for v12 and v11? I wonder how we are going to maintain x-teaser's components (x-teaser, x-teaser-list and x-teaser-timeline).
There was a problem hiding this comment.
There hasn't been much activity in the x-teaser package in the last few years so I don't expect there to be any bug fixes that will require changes in both versions, but you're right that it's a concern and we'll (the platforms team) be happy to backport any bug fixes if the need does arise.
There was a problem hiding this comment.
Hey @ivomurrell, I'm sorry but this should not be shipped as a breaking change but as an optional feature. For the time being the ES data format should be the default and an opt-in switch should enable an adapter for the new format. Are you able to make those change to this PR?
Switching from Next ElasticSearch to another API is, for the time being only happening within The article body and forcing an upgrade to data is way out of scope of API rationalisation, which is replacing the article body architecture. Moving away from ES is a very large project without analysis or an adequate alternative.
There are many call sites that rely on, read and manipulate this data format across many systems, whether that be presentation logic using x-teaser or something else that reads/ manipulates teaser data. We're not changing that code in the foreseeable future and meanwhile we need to keep the latest version of the teaser component not only usable everywhere but something we can iterate on separately from the migration initiative.
The shape of x-teaser's props were historically designed to match the shape of data that was stored in ElasticSearch. Now the data's shape is declared in
cp-content-pipeline-schemaas a GraphQL schema, and we should update the x-teaser properties to match. This will allow us to remove a hack that was previously in place incp-content-pipeline-schemato support both the newContenttype (which will subsequently be updated to provide all the metadata x-teaser needs) and the legacyTeasertype.This is a breaking change for the component, which we can roll into one major release (v12), along with the changes to x-gift-article. We'll add a wiki page detailing how to migrate both of the components.