feat(project): services modularization

[AntonLantukh]

Controllers and Services

Controllers and Services can both be used to provide services (objects) that can be injectable into parts of the application.

Services - domain related entities. Business logic is stored there. We use services to communicate with the back-end, to process the data we receive. Services help to manage different dependencies. For example, we can use them to support several integration providers or streaming schedule providers. If this is the case we should also create a common interface and make dependant entities use the interface instead of the actual implementation. This is how inversion of control principle can be respected. Then when we inject services into controllers we use interfaces types instead of the implementation classes.

• They contain the actual business logic to perform the use case;
• They can be injected into controllers which orchestrate different services;
• They should not used directly in the View / UI;
• One service can use different implementation. For example, we can split it into cleeng and jw implementation (account, checkout and so on).

Controllers - binding different parts of the application, using services, store and providing methods to operate with business logic in the UI and in the App. If we need to share code across controllers then it is better to promote the code to the next level. Then it is possible to modify both controllers to call the same (now shared) code. We should try to avoid controllers calling each other because it leads to circular dependencies and makes the code messy. However now they do it sometimes.

• They can be called from the View part of the application;
• They can use a Store to find the necessary data;
• They use the data from the Store and from the UI to operate different injected services;
• They use a Store to persist the entities again;
• They return data back to the UI if needed.

Store - If the code is related to storage/retrieval, it should go in the Store. Both controllers and UI / View can use Store in accordance with their needs. Something to think about: use classes and inject them into controllers.

---

InversifyJS

InversifyJS library is used to provide IOC container and to perform dependency injection for both services and controllers. Injection happens automatically with the help of the reflect-metadata package.

initDefaultServices function is used to init default services and controllers which don't depend on any integration provider.

In the initIOCData function we initialise controllers based on the selected integration provider and inject necessary services.

Steps completed:

According to our definition of done, I have completed the following steps:

• Acceptance criteria met
• Unit tests added
• Docs updated (including config and env variables)
• Translations added
• UX tested
• Browsers / platforms tested
• Rebased & ready to merge without conflicts
• Reviewed own code

[github-actions]

Visit the preview URL for this PR (updated for commit e73e05c):

https://ottwebapp--pr363-feature-services-mod-smampy9r.web.app

_{(expires Sat, 16 Dec 2023 07:59:09 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c198f8a3a199ba8747819f7f1e45cf602b777529}

[dbudzins]

I tried to comment on the major suggestions for this refactor, but 1 other point I want to make as well is I think it's best if we don't do file moving / renaming with this change. There is already going to be enough churn to cause merge conflicts without also trying to trace conflicts across file paths. Then we can follow up this PR with a big move / rename that doesn't have any logic changes. I think that will make merges much less painful.

[ChristiaanScheermeijer]

Nice work @AntonLantukh, this is huge!

This is a great step forward! This will make it easier to separate the logic from the UI and deal with the multiple integrations better.

I do have one generic comment regarding the actual separation of integrations. The UI is mainly based on the data/logic from Cleeng APIs. This requires the JWP integration to transform all data to the Cleeng format. I would like proof that the UI works using OTT app-only models and types. The services would need to transform the data into an OTT app model used by UI, controllers and stores.

For example;

type User = {
  id: string;
  email: string;
  profile: {
    firstName?: string;
    lastName?: string;
  }
  // ...
};

export default interface AccountService {
  // ...
  getUser: ({ config }: { config: Config }) => Promise<User>;
  updateUser: ({ config, user }: { config: Config, user: User }) => Promise<User>;
  // ...
}

[dbudzins]

I just created a PR on your PR with a working version of the reflect stuff for using classes as identifiers w/o the need for @inject everywhere: #368

If you move all the files back to match the structure currently in develop and merge to get latest, the number of files changes and diffs should drop down a bunch.

[AntonLantukh]

Some draft comments:

1. useClientIntegration hook should be part of the config store. This one is just basically getting data from the store
2. SettingsStore and SettingsController can be removed in favour of pre-compile logic. It will let us stop triggering file download on every page load + helps to use Config as a Service + Controller
3. In order for this system to work we need to import dependant services and controllers as classes and not as types. This one can be tricky and took me first time to resolve it as the inversify error was not really clear.

Doesn't work:

import type AccountService from '#src/services/account.service';

@injectable()
export default class WatchHistoryController {
  private readonly accountService: AccountService;

  constructor(accountService: AccountService) {
    this.accountService = accountService;
  }

Works:

import AccountService from '#src/services/account.service';

@injectable()
export default class WatchHistoryController {
  private readonly accountService: AccountService;

  constructor(accountService: AccountService) {
    this.accountService = accountService;
  }

[ChristiaanScheermeijer]

Nice changes @AntonLantukh!

Have you also considered factories or dynamic values to achieve dynamic integrations?

For example, you can use named factories like so:

container.bind<AccountService>('AccountService').to(CleengAccountService).whenTargetNamed('cleeng');
container.bind<AccountService>('AccountService').to(InPlayerAccountService).whenTargetNamed('inplayer');

// Factories
container.bind(AccountServiceFactoryId).toFactory((context) => {
  return (integration: string) => integration ? context.container.getNamed<AccountService>('AccountService', integration) : undefined;
});

// usage
class AccountController {
  constructor(
    @inject(AccountServiceFactoryId) accountServiceFactory: AccountServiceFactory,
  ) {
    const { getAuthProviderName } = useConfigStore.getState();

    this.accountService = accountServiceFactory(getAuthProviderName()); // optional
  }
}

Or using a dynamic value that resolves the integration:

container.bind('CleengAccountService').to(CleengAccountService);
container.bind('InPlayerAccountService').to(InPlayerAccountService);

container.bind(AccountService).toDynamicValue((context: interfaces.Context) => {
  const config = context.resolve(ConfigService);
  const authProvider = config.getAuthProviderName();

  if (authProvider === 'jwp') return context.resolve('InPlayerAccountService');
  if (authProvider === 'cleeng') return context.resolve('CleengAccountService');
});

[AntonLantukh]

Nice changes @AntonLantukh!

Have you also considered factories or dynamic values to achieve dynamic integrations?

@ChristiaanScheermeijer Thanks! It looks like it may add some complexity in the init logic (dynamic value approach). For AccountService and CheckoutService we may also need CleengService. So it would result in something like this (we also add unnecessary service-specific injectables):

  container.bind(CleengAccountService).to(CleengAccountService);
  container.bind(InplayerAccountService).to(InplayerAccountService);
  container.bind(CleengCheckoutService).to(CleengCheckoutService);
  container.bind(InplayerCheckoutService).to(InplayerCheckoutService);

  if (integrationType) {
    if (integrationType === INTEGRATION.CLEENG) {
      container.bind(CleengService).toSelf();
    }

    container.bind(AccountService).toDynamicValue((context: interfaces.Context) => {
      if (integrationType === 'JWP') {
        return context.container.resolve(InplayerAccountService);
      }

      return context.container.resolve(CleengAccountService);
    });

    container.bind(CheckoutService).toDynamicValue((context: interfaces.Context) => {
      if (integrationType === 'JWP') {
        return context.container.resolve(InplayerCheckoutService);
      }

      return context.container.resolve(CleengCheckoutService);
    });

    // ....
  }

The approach with factories requires additional factory types like 'AccountServiceFactory'. We removed string and symbol based types in favour of native classes / types (though we may reconsider it). Cleeng also needs to be initialized somehow (additional condition?):

init.ts:

  const { integrationType } = configController.getIntegration();

  container.bind<AccountService>(AccountService).to(CleengAccountService).whenTargetNamed(INTEGRATION.CLEENG);
  container.bind<AccountService>(AccountService).to(InplayerAccountService).whenTargetNamed(INTEGRATION.JWP);

  container.bind('AccountServiceFactory').toFactory((context) => {
    return (integration: keyof typeof INTEGRATION) => (integration ? context.container.getNamed<AccountService>(AccountService, integration) : undefined);
  });

  if (integrationType === INTEGRATION.CLEENG) {
    container.bind(CleengService).toSelf();
  }

AccountController:

@injectable()
export default class AccountController {
  private readonly checkoutService: CheckoutService;
  private readonly accountService: AccountService;
  private readonly subscriptionService: SubscriptionService;
  private readonly favoritesController?: FavoritesController;
  private readonly watchHistoryController?: WatchHistoryController;

  constructor(
    checkoutService: CheckoutService,
    // accountService: AccountService,
    @inject('AccountServiceFactory') accountServiceFactory: (integrationType: 'JWP' | 'CLEENG' | null) => AccountService,
    subscriptionService: SubscriptionService,
    @optional() favoritesController?: FavoritesController,
    @optional() watchHistoryController?: WatchHistoryController,
  ) {
    this.checkoutService = checkoutService;

    const { integrationType } = useConfigStore.getState().getIntegration();
    this.accountService = accountServiceFactory(integrationType);

    this.subscriptionService = subscriptionService;
    this.favoritesController = favoritesController;
    this.watchHistoryController = watchHistoryController;
  }

[ChristiaanScheermeijer]

Ah, I hoped we could move the DI init logic to the index.ts. With dynamic values or factories, we don't depend on knowing the integration type when registering all classes.

This means:

• All integration classes are always bound to the container
• Inside the dynamic value (I think this is performed when injecting the value, right?), we resolve the integration type and correct service dynamically
• The application doesn't care which integration is used. It expects an AccountService or undefined when no integration is configured.
• container.bind(CleengService).toSelf(); can always be bound. We are never requesting this when Cleeng isn't configured

We may want to do code splitting based on the configured integration. E.g., we don't want to load the InPlayer SDK for AVOD or when Cleeng is used (and vice-versa).

But we must first lazy import SDKs in the initialize function of the service instead of importing them directly.

Factories seem the logical feature for this, but I also don't like injecting factories/symbols instead of the class directly. That's why I hoped the dynamic value could be used similarly to regular classes.

I experimented with this branch and refactored a few things to make this work (I used factories here):

• Added ConfigController, which loads and validates the config but also determines the authProviderName and stores it
• Added ApplicationController, which bootstraps the application:
  • await ConfigController#loadConfig
  • await AccountController.initialize()
    • this.accountService = accountServiceFactory(authProviderName);
• <Root /> calls ApplicationController.bootstrap()

[AntonLantukh]

@ChristiaanScheermeijer @dbudzins I merged latest changes from develop and additional branch we had.

1. I added one error with the payload. Depending on the current env we can either just show it "as is" or use it together with some dev / preview components;
2. Features are now part of the controller;
3. Some modules use 'not required' resolution (profiles);
4. Unnecessary controllers were removed;

Thoughts?

[ChristiaanScheermeijer]

Nice work @AntonLantukh! I think the PR is finished and can be merged 🎉

One last question; do you know why the two e2e tests are failing, is that caused by this PR or were they already broken?

[dbudzins]

Do you need different components here? Won't IS_DEMO_OR_PREVIEW take care of all the differences?

[AntonLantukh]

I think I prefer to understand which 'side components' our main components will have depending on the environment. Here we can clearly differentiate between prod and other envs and can probably also easily remove the second part when not needed.

[dbudzins]

I would rather export a default method and actually call it, instead of relying on side effects of an import.

[AntonLantukh]

We should 'import' and bind modules before App is imported (and our AppController is invoked). If we invoke it here, then it starts to fail, as Import statements are always hoisted to the very top.

[dbudzins]

I still don't think this needs to be a separate hook instead of just being inline in Root.tsx.

[AntonLantukh]

It also helps to separate typings from the hook's response in reuse them in DemoConfigDialog

[dbudzins]

A few small suggestions, but otherwise it looks GREAT!

[AntonLantukh]

Fixed tests, asked Cleeng about the coupon changes.