Discussion about how to improve page rendering and simplify components

[jroebu14]

I was thinking about a better way to render page components that would remove a lot of logic from our components.

TLDR:

• Stop passing complex and unprocessed data structures that come from API responses to components and do the data processing in getInitialData.
• We shouldn't design components around unprocessed data structures from APIs in case the data structures change.
• Only pass components data they need to fully render. This will also make prop types easier to manage and read.

Here is something I scribbled down to demonstrate how a page could be rendered.

I also asked Sareh what she thought about it and took relevant bits from our discussion and pasted below.

Keen to hear what others think about this and any ideas they might have.

const getInitialData = () => {
  // do a data fetch
  // process data the page needs before sending data to page component
  return {
    author,
    firstPublished,
    lastPublished,
    section,
    aboutTags,
    mentionsTags,
    blocks,
    language
    // absolutely everything else a page needs in the flattest possible structure
  };
};

const ThePageComponent = ( //e.g. will be <FrontPage>, <Article>, <MediaAssetPage> etc
  pageData // page data comes in here in a way more sensible and flatter structure that is much easier to deal with
) => (
  <main>
    <ATIAnalytics // components should be not be designed around API response data structures and should only be passed the props they need
      {...pluck(
        // using ramda you can pluck exactly the data the component needs from pageData
        // or if you think pluck is ugly then destructure instead
        [
          'publisherId',
          'language',
          'section',
          'blah',
          'otherThing',
          'moreStuff'
        ],
        pageData
      )}
    />
    <Metadata // no Container suffix to the component name here. The container pattern is for managing state and data fetching. We already do data fetching in `getInitialData` so we should do data processing there too and not pass complex data structures around components
      // metadata used to accept a prop called `promo` only because it came from an Ares response. It doesn't mean anything in the context of metadata. it should accept only what it needs
      {...pluck(
        [
          'firstPublished',
          'lastPublished',
          'aboutTags',
          'blah',
          'otherThing',
          'moreStuff'
        ],
        pageData
      )}
    />
    <OtherComponet
      {...pluck(['publisherId', 'language', 'section'], pageData)}
    />
  </main>
);
PageComponent.propTypes = {
  // use the prop types defined on the child components instead of defining them twice or having a centralised props folder
  ...ATIAnalytics.propTypes, // prop types will be way less complex because we processed the data into something sensible in `getInitialData`
  ...Metadata.propTypes,
  ...OtherComponet.propTypes
};

S:
In this getInitialData will there be different functions called e.g. optimoArticle cpsFrontPage, cpsMapPage? To return the relevant data fields.

J:
it basically does data processing for the whole page in getInitialData instead of passing complex data structures around components

S:
Okay.

J:
but I prefer getOptimoArticleData getCpsFrontPageData, getCpsMapPageData naming
then those can be unit tested very easily

S:
We discussed this a while ago. 🙂 Ended up not doing it since there was only a single pagetype.
Where would you handle the preprocessor step?
It’d be good to have that happen up-front here if possible
e.g. see all the rules here https://github.com/bbc/simorgh/tree/latest/src/app/lib/utilities/preprocessor/rules

J:
yeh we could keep that bound to the fetch function
but tbh I think it's not needed

S:
but tbh I think it’s not needed
How come?

J:
sorry
the functions are still needed of course
but they could just be imported and used like normal functions in getInitialData
and composed using compose or pipe
but I wouldn't want to complicate the idea so I'd leave it the way it is for now

NB: I was talking about how the preprocessorRules arg in fetchData does the same as what ramda pipe does

  return fetchData({
    url,
    preprocessorRules: [
      applyTimestampRules,
      addIdsToBlocks,
      applyBlockPositioning,
    ],
  });

is the same as

  return fetchData({
    url,
  }).then(
    pipe(
      applyTimestampRules,
      addIdsToBlocks,
      applyBlockPositioning,
    )
  )

S:
Okay. I’m thinking that the getOptimoArticleData getCpsFrontPageData, getCpsMapPageData are quite similar in functionality to preprocess in '#lib/utilities/preprocessor' - the first group of functions return values that are needed. preprocessor adds/moves around/normalises the data.
They’re just different forms of data manipulation post-fetch
So I think it’d be good if they happened consecutively or in the same sequence of commands.
I don’t mind where exactly that’d happen

J:
OK I see your point

S:
I think conceptually they make sense happening right after one another

J:
Yeh you're right

S:
Might even be easier to update the preprocess commands to be more useful here -

S:
to do the flattening of the feed for optimo /cpsMap/etc
so we don’t have two different types of processing - just one type, that is organised quite clearly

J:
ok cool. my worry is that it can be tricky to pass raw data through a sequence of functions composed together and get exactly what you want out the other end

S:
That’s true.

J:
it's sometimes way easier to store the result of functions in variables and then spread the variables onto an object

S:
Yes, I agree. There are so many different fields though - and so much nesting - that I think it could be difficult to keep on top of.
You can see all the different values passed into Metadata (component and container)
There are so many more for each pagetype and that’ll increase with every new pagetype we have
I think it’ll be a difficult task to keep on top of all of the functions in variables in one place.

J:
I think it's better to have in one place because if an API data structure changes then we change it one place rather than in all the components that are passed the pageData

S:
It’ll just push the complexity up the chain out of containers and into a place that is kind of abstract - divorced from the implementation and therefore difficult to know if a change in a function will break the functionality on the front end. e.g. thinking of how fields from all over the feed can be used for tags.
That said, we have had similar layers in the past and they have worked in simplifying front-end data manipulation.
I think it’s better to have in one place because if an API data structure changes then we change it one place rather than in all the components that are passed the pageData
Yeah, that’s fair

J:
I just feel that every single component is quite complex
in simorgh that is

S:
I agree with you on that.

J:
it would be cool if we thought about how to describe a page type in one object and then process data fetches into that object

S:
I don’t think the abstraction will be quite so tidy.
But I do think it’d be worth exploring.

J:
ok cool. could be one for this learning day

[chris-hinds]

@jroebu14 where you have PageComponent in your example is this still assuming we would have multiple top level page components i.e. <FrontPage>, <Article>, <MediaAssetPage>

[jroebu14]

@hindsc52 correct. I'll change that in the example to prevent confusion.

[pjlee11]

I think this is a good idea. For Articles we previously spoke about flattening the Optimo feed but this didn't seem to have much benefit at the time. Now that Simorgh is receiving multiple data structures flattening them into a consistent format would make the container logic much simpler and remove the need for structure specific logic in the containers/components

[ryanmccombe]

This looks good - we should do this

We had previously suggested doing something similar for media asset page work, ie defining a simorgh-specific schema optimised to our needs, and conforming the various formats we get from the different APIs to that one standard

It was shot down, but I gather sentiment has warmed to doing these types of conversions. We're already planning to conform block schemas at the getInitialData stage - why not do the same to the rest of the data too?