Made sectionName optional

[rhystmills]

What does this change?

This PR makes the sectionName property in the Targeting type optional.

The sectionName property in the Targeting type doesn't appear in the ReaderRevenueBanner payload on DCR (which, based on testing, doesn't seem to be under type control currently: once we resolve the type discrepancies we can change this), and is optionally an empty string in SlotBodyEnd.

A related PR is the removal of the ophanComponentId property from the tracking type.

N.b. this PR formerly added isSensitive as a prop, but it's not actually used on the platform so I will remove it there instead.

[tjmw]

I left a couple of comments on the Targeting type changes made here. I'm not 100% on their usage though.

@tomrf1 I think your input here would be valuable, it you have a moment to look at these changes?

[tjmw]

Interestingly this is required for epic targeting according to the type definition here, though like you say not required by banner targeting here.

So it's true that the net result if we were to combine the types is that it's optional, though I think that doesn't quite reflect the reality. I wonder if a better place to end up would be to split this type into two (one for the epic, one for the banner) like support-dotcom-components does. Not suggesting you have to take on that refactoring here though! Also interesting to think about how to reduce the duplication here and in support-dotcom-components (these types should always be aligned I think).

In the meantime perhaps making this optional as you've done is the most pragmatic way forward.

[rhystmills]

Splitting the two sounds like a good decision to me.

If the 'canonical' types are in support-dotcom-components, could we just import them from there?

[tomrf1]

It looks like this Targeting definition is only used by the epic (SlotBodyEnd) - is that right?
What does automat-client give us?

[rhystmills]

Yeah, it's enforced (not particularly explicitly) by the use of getBodyEnd from automat-client, which uses the Metadata type which includes Targeting and Tracking.

[rhystmills]

From testing, I don't think ReaderRevenueBanner has any type control at all (you can change any of the properties without a complaint from Typescript) - I think ideally it would use the types from elsewhere though - the problem at the moment is that the types it includes aren't included in the definitions from automat-client. From what Tom W says, perhaps there are more canonical types in support-dotcom-components?

Perhaps both DCR and automat-client should be importing types from there?

[tjmw]

What does automat-client give us?

Reminding myself by looking over the repo. It looks like the main things are:

• type definitions (though as we're discovering, it seems like this is a bit over-simplified and perhaps it'd be better if we could somehow share the type definitions from support-dotcom-components).
• a thin wrapper around fetch for the end of article api call (getBodyEnd)
• some shared libs around views retrieving and storing message views (stored in local storage)

So it seems like if we wanted we could move towards embracing automat-client more. For example getBanner which is the equivalent of getBodyEnd is currently implemented directly in ReaderRevenueBanner. Perhaps we want to move that to the client. But then if that's not been a pain point, maybe automat-client isn't pulling its weight in terms of usefulness to warrant having this additional lib.

I do think sharing the type definitions in support-dotcom-components is the way to go. Perhaps there's a way to achieve that with a monorepo approach, where the support-dotcom-components repo also includes an NPM lib (which could replace automat-client). It seems like there might be less friction that way.