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[bot]]

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}

[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: https://github.com/jwplayer/ott-web-app/pull/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]

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()

@ChristiaanScheermeijer Do you have a draft PR with a possible implementation? It should not be a working one, just to see the concept. We already have an initApp function which does all the job (init services / controllers + start the app). Do you suggest to split this part into:

1. Init services (Probably Root component) using factories, bind all the services and controllers we have. Don't add any conditions for binding based on the config value.
2. Start the app (Probably RootLoader) in the ApplicationController ?

[ChristiaanScheermeijer]

Hi @AntonLantukh, I've extracted the important parts in a Gist:

https://gist.github.com/ChristiaanScheermeijer/72ba106b1cf21c041a2c32c5b04be1c8

The most important thing to remember is that a integration services can not be loaded outside a component.

I actually would like to do this:

const accountService = getModule(AccountService);

const FavoriteButton = () => {
  return <div />;
};

But instead, the module needs to be resolved inside the render function:

const FavoriteButton = () => {
  const accountService = getModule(AccountService);
  return <div />;
};

Otherwise, the module will be resolved before the config is loaded. We can probably safe guard this by throwing an error in the toDynamicValue functions.

[ChristiaanScheermeijer]

@AntonLantukh I've updated the Gist with the dynamic part of the services DI.

There is a difference in concept between our implementations.

• In the current implementation, services and controllers may or may not be registered, and that mechanism is used to toggle certain features. That will not work with my proposal, as all services are registered.
• The bootstrapping and initialization are a bit scattered in the current implementation. I felt the urge to unify this and do the bootstrapping in a dedicated controller and service.
• IMO the Root component shouldn't be knowing about the implementation details of accounts and user profiles. This should be initialized in the controllers and services.

Thinking ahead to make it also possible to use this architecture for non-browser apps, putting the containers file in the src folder would make it convenient for replacing certain services/controllers with different implementations.

[AntonLantukh]

@ChristiaanScheermeijer @dbudzins Hey guys! I combined Settings and Config controllers into one AppController which is responsible now for getting all the needed resources to init the app.

Then I also prepared two possible implementations of how we init the app (there are init1 and init2 + AppController1 and AppController2 files). By default second approach is used.

1. We bind services / controllers in one function + init the app hitting favourites, account and wathhistory endpoints.

Pros:

1. We bind controllers conditionally and make it explicit which of them are not supposed to be used depending on the case (integration or anything else).
2. We have one entry point and can easily track all the init logic.

Cons:

1. One function is responsible for binding and bootstrapping. Binding and bootstrapping operations are coupled.
2. We don't have access to controllers / services outside of react components.

2. We use dynamic values.

Pros:

1. We have access to services / controllers outside of react components. Though I am not sure whether we should address controllers before the app is initialised.
2. We split binding and init into two pieces which can work independently.
3. Non-browser apps (?). @ChristiaanScheermeijer could you please elaborate on this?

Cons:

1. We should use controllers in AppController bypassing DI (getModule) because we don't have access to Config / Settings when instantiating AppController.
2. The whole init flow is split into several files We should import one file in index.ts and one in Roots.tsx. It could lead to the higher barrier of entry.

Thoughts?

[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?

[AntonLantukh]

Fixed tests, asked Cleeng about the coupon changes.