search

Iterable / iterable-web-sdk

Iterable SDK for interacting with the Iterable API to implement inside JavaScript and Node projects
https://iterable.com
MIT License
9 stars 9 forks source link

Too many Embedded-related interfaces? #359

Closed hardikmashru closed 5 months ago

hardikmashru commented 5 months ago

JIRA Ticket(s) if any

Description

Test Steps

mprew97 commented 5 months ago

I still see some types that could be consolidated, i.e., IEmbeddedImpression -> Impression, IEmbeddedSession -> Session. Mind doing an audit of these to see if some of these types are redundant?

And one nit, can we remove the I prepended on the interfaces (IEmbeddedMetadata -> EmbeddedMetadata)? We know the type is an interface so it's a bit unnecessary.

Thanks in advance!

hardikmashru commented 5 months ago

I still see some types that could be consolidated, i.e., IEmbeddedImpression -> Impression, IEmbeddedSession -> Session. Mind doing an audit of these to see if some of these types are redundant?

And one nit, can we remove the I prepended on the interfaces (IEmbeddedMetadata -> EmbeddedMetadata)? We know the type is an interface so it's a bit unnecessary.

Thanks in advance!

Some types of interfaces could not be consolidated like IEmbeddedSession -> Session because IEmbeddedSession has start, end, placementId, impressions, id and deviceInfo Session has id, start and end.

IEmbeddedSession is used in trackEmbeddedSession in src/events/embedded/events.ts. Session is used internally in EmbeddedMessagingSession interface in src/events/embedded/types.ts

We cannot remove the 'I' prefix from the interfaces because it will result in the same name as some classes. For example, There is an EmbeddedImpressionData class, and the IEmbeddedImpressionData interface is used in src/embedded/embeddedSessionManagers.ts.

EmbeddedSession class, and the IEmbeddedSession interface is used in src/embedded/embeddedSessionManagers.ts.

So to remove I from the interface we have to think about new names for those interfaces which right now seem like ideal names.

mprew97 commented 5 months ago

I still see some types that could be consolidated, i.e., IEmbeddedImpression -> Impression, IEmbeddedSession -> Session. Mind doing an audit of these to see if some of these types are redundant? And one nit, can we remove the I prepended on the interfaces (IEmbeddedMetadata -> EmbeddedMetadata)? We know the type is an interface so it's a bit unnecessary. Thanks in advance!

Some types of interfaces could not be consolidated like IEmbeddedSession -> Session because IEmbeddedSession has start, end, placementId, impressions, id and deviceInfo Session has id, start and end.

IEmbeddedSession is used in trackEmbeddedSession in src/events/embedded/events.ts. Session is used internally in EmbeddedMessagingSession interface in src/events/embedded/types.ts

We cannot remove the 'I' prefix from the interfaces because it will result in the same name as some classes. For example, There is an EmbeddedImpressionData class, and the IEmbeddedImpressionData interface is used in src/embedded/embeddedSessionManagers.ts.

EmbeddedSession class, and the IEmbeddedSession interface is used in src/embedded/embeddedSessionManagers.ts.

So to remove I from the interface we have to think about new names for those interfaces which right now seem like ideal names.

@hardikmashru understood! thanks for the thorough response!