Describe and enforce patterns in our providers better (with TypeScript)

[artfuldev]

Describe and enforce patterns in our providers better (with TypeScript)

[artfuldev]

Also, typically in TypeScript we don't use an I prefix to denote interfaces for several reasons. I'd also like to understand the reasoning behind that choice, if any. I'm happy to outline the arguments to not use an I prefix if required.

[artfuldev]

I would like to understand what we think of this so we can decide if it can be picked up.

I would probably create sub-issues and use this as a tracking issue of sorts if we're gonna pick this up.

[ComBarnea]

would love to hear @scopsy thoughts about this as well, I definitely tend to agree with most points, and due to the nature of points :), I'll answer the same.

1. Agreed, easier to read, and provides a more consistent API.
2. Not so many thoughts around this one, mutating options inputted to the provider is bad behavior.
3. Absolutely love this one, organize types of input options in proper names! We do need to keep in mind that some naming conventions should be enforced in order to create a unified API, and looking into API-based providers, this could be a bit confusing for library users. For example

const provider = new SendgridEmailProvider({
  apiKey: process.env.SENDGRID_API_KEY
});
// actual API key name is apiKey

const provider = new PostmarkEmailProvider(we
  apiKey: process.env.POSTMARK_API_KEY
});

// while here actual API key name is serverToken

Due to these naming issues across providers, we should normalize and formalize names, and just as you started here create boilerplate templates for each.
4. Agreed, @scopsy had some conversations around that, and we wanted to see if it's just us, or will the community will think the same, that being said, we need to make sure it's as easy, and DX for contributors is good.
5. Email might be talked care of, we still lack all the others.
6. Nothing to add here.

All in all, I really like these suggestions, we need to think a bit more about some of these, and I'd definitely want to see @scopsy and other @notifirehq/notifire-contributors have to say about that.

[scopsy]

Thanks for the detailed post, I'll write the comments directly below:

1. Could you describe the benefits of using this over the previous solution? If it has clear advantages we definitely can go that way.
2. Good idea, let's do that.
3. We can go that direction, however, I believe there will be some exceptions. Love the standardization of the inputs, but we will for sure have some provider-specific configs that could not be normalized.
4. Technically that's correct, I believe for newcomers it might be a little confusing. We can introduce it a bit down the road once we have more providers' variety.
5. We can change some of the non-extendable interfaces to types.

I'll open relevant issues and attach them here. Thank you @artfuldev

[artfuldev]

Thanks @ComBarnea and @scopsy for your consideration and thoughtful comments.

Regarding:

1. 😅 sorry I didn't make it clear. It just makes the type-dependency explicit.

For example, I can type a KeyValuePair as:

interface KeyValuePair<K, V> {
  key: K;
  value: V;
}

or as:

// type K = any | all possible types
// type V = all possible value types
interface KeyValuePair {
  key: K;
  value: V;
}

The first one just makes it clear that the type dependency exists. The second reason is extending such an interface means writing much less code. In our provider case,

export interface IProvider {
  id: string;
  channelType: ChannelTypeEnum;
}
export interface IEmailProvider extends IProvider {
  channelType: ChannelTypeEnum.EMAIL;
  sendMessage(options: IEmailOptions): Promise<IWhatever>;
}

If we made the type dependency explicit, it's much simpler:

export interface IProvider<C extends ChannelTypeEnum> {
  id: string;
  channelType: C;
}
export interface IEmailProvider extends IProvider<ChannelTypeEnum.EMAIL> {
  sendMessage(options: IEmailOptions): Promise<IWhatever>;
}

Notice that we didn't have to specify the channelType property at all, as it's clear from the interface we extend already. There's also sufficient type constraints provided which serves as self-documenting. Anyway it's a simpler, more explicit, more self-documenting, stricter-typed, terser version of the same interface definition. That's all. It's not functionally any different.

[artfuldev]

@scopsy @ComBarnea What are the issues we want to create out of this? We can probably just edit the issue to be simpler and convert this into a tracker issue of sorts. What do you think?

[artfuldev]

Also @scopsy point 3 is meant to be extensible, we're not getting locked into anything. We kind of have an empty EmailProviderConfig interface 😅

[scopsy]

@artfuldev Yeah we definitely can use this issue to be a tracker. I'll try to review this tomorrow and write the action items to develop from it. Thanks!

[scopsy]

A lot has changed since :) Closing this one as we have moved more toward an HTTP API approach. Thanks everyone involved here