task(email-renderer): Add types to email template partials

[dschom]

Because

• We can also declare types for partial templates

This pull request

• Declares types for partial templates
• Updates current templates to union types defined by partial templates

Issue that this pull request solves

Closes: FXA-12613

Checklist

Put an x in the boxes that apply

• My commit is GPG signed.
• If applicable, I have modified or added tests which pass locally.
• I have added necessary documentation (if appropriate).
• I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

[LZoog]

Started review, but need to review another PR for the tag and will circle back to finish.

[LZoog]

Should we have SubPlat take a look at this PR as well? Wondering if some things might not be optional values, but if these are pulled from the templates themselves then makes sense.

[dschom]

It'd be great to a get a second set of eyes on this. Making them optional was most straightforward, however you could get really fancy and do compound types. A lot of the logic in the templates checks for the presence of a single field, and if that is there then it expects the other fields will be too.

Here's some psuedo code showing the two ways you could do this:

const template = `
    <h1><%- title %></h2>
    <% if (!!invoiceSubtotalInCents) { %>
        <p>Your total is: <%- invoiceSubtotal %></p>
    <% } %>
`;

// An example of loose typing. Developers still know what the options are, but it's up to them to supply them.
// Pros: Easy to maintain, Somewhat typesafe.
// Cons: Have too look at the template.
{
    type templateDataLoose = {
        title: string,
        invoiceSubtotalInCents?: number,
        invoiceSubtotal?: string,
    }
    const foo:templateDataLoose = { title: 'foo', invoiceSubtotal:undefined };
    const bar:templateDataLoose = { title: 'bar', invoiceSubtotal:'$0.10', invoiceSubtotalInCents: 10 };
    const baz:templateDataLoose = { title: 'baz', invoiceSubtotal:'$0.10' }; // Loose typing allows this
}

// An example fo strict template typing
// Pros: Potentially very type safely
// Cons: Hard to maintain, Harder to read, TypeDefs themselves might become confusing. 
{
    type TemplateDataStrict = {
        title:string
    } & (
        {
            invoiceSubtotalInCents?: undefined,
            invoiceSubtotal?: never,
        } | 
        {
            invoiceSubtotalInCents: number;
            invoiceSubtotal: string,
        }
    );

    const foo:TemplateDataStrict = { title: 'foo' };
    const bar:TemplateDataStrict = { title: 'bar', invoiceSubtotalInCents: 10, invoiceSubtotal:'$0.10',  };
    // However, these objects would result in typescript errors
    const baz:TemplateDataStrict = { title: 'baz', invoiceSubtotalInCents: 10 }; // A more strict typing can prevent this kind of issue
    const biz:TemplateDataStrict = { title: 'baz', invoiceSubtotal: '$0.10' };
}

I think at the end of the day you still need to look at the template. Now that I wrote this little example I must admit I do like the TypeSafety of the second approach. Would love second opinions though, on if its worth the extra effort.

[dschom]

@LZoog Just pinging you again on this one for feedback.

[LZoog]

@dschom this is a hard call lol. My first reaction was that I like the second approach but that I could also see it feeling messy or confusing at first glance. It's annoying to have no typesafety in emails.js and then also have them so loose in the templates themselves, so my first thought was the strict type safety is a nice improvement in the templates at least, but, looking at it again, I guess I'm also torn on if it's worth the level of effort, and how much it will actually help us in development or in clarity over option 1. I'm thinking we go with the first approach for now, and circle back to the stricter option later? I'm open to others' opinions as well.

[dschom]

@LZoog Thanks for weighing in. I think just having the template variables clearly specified, optional or not, by types is a huge win. How helpful the the stricter type definitions would end up being seems a bit uncertain to me.

If we go with option 1, which is definitely less work up front, then we can always keep option 2 in our back pocket. In the event we see these loose types causing mistakes or oversights, there's also no reason we can't apply stricter types where necessary. e.g. Maybe there's a template with so many optional types it becomes confusing or error prone. In this is the case, we can refine the type definition for that specific template.

[LZoog]

Sounds great to me 👍

[LZoog]

Just curious, since we have lots of values/types that are the same (e.g. this one's used in a ton of templates), do you think it's worth having a follow up to share these types more closely? Wondering even if it makes sense to give the partials names like type AutomatedEmailNotAuthorized, and then the template type can reference/pull in the same names as the partials its using.

[dschom]

That might be nice. Overall it seemed consistent with how the /templates/index.ts files have been structured. It also hasn't been much a problem since it's really easy to re-reference these types on import, ie

{ TemplateData as AutomatedEmailNotAuthorizedTemplateData } from '../partials/automatedEmailNotAuthorized'`;

But refactoring is just a button click, so I think it's fine to do this if we want to.

[dschom]

@LZoog Just pinging you again on this one.

[LZoog]

I suppose I've got a slight preference for just naming it AutomatedEmailNotAuthorizedTemplateData if we're always going to rename the import since it also makes it nicer to just search the repo for the same name, but if you feel like it's more consistent this way I'm fine with that too. The opinion is not a strongly held one :)