scottohara / tvmanager

PWA for tracking recorded, watched & upcoming TV shows
MIT License
4 stars 0 forks source link

ApplicationController should be a singleton service #83

Open scottohara opened 6 years ago

scottohara commented 6 years ago

Anywhere we need a reference to the app controller, we create a new instance of one, e.g.

const appController = new ApplicationController();

However, under the covers, it actually behaves like a singleton object. For example:

const appController1 = new ApplicationController();
const appController2 = new ApplicationController();

console.log(appController1 === appController2);  // true

(it does this by storing a reference to itself in the private static singletonInstance property, and the constructor returns this reference on the second and any subsequent calls)

Given this, and the fact that the *Controller naming convention is intended for 'view controllers' (i.e. classes that extend ViewController), it would be more appropriate to:

  1. Rename the class from ApplicationController to ApplicationService
  2. Move it from /controllers/application-controller.ts to /services/application-service.ts
  3. Remove the constructor (and move anything there into start())
  4. Make all instance properties & methods static
  5. Remove all local references to ApplicationController instances in other classes, and simply reference ApplicationController as a static singleton service.
scottohara commented 2 years ago

In @typescript-eslint@5.18.0, the no-this-alias rule was improved to catch more assignment of this.

As such, we started to get warnings about ApplicationController.singletonInstance = this;

To address, we have temporarily disabled the rule, but will look to re-enable after changing from a singleton-instance to a service.

(Note: there is one other instance of this rule violation in spec/public/mocks/window-mock.ts, where this.window = this. This may mean we need to keep the rule disabled via spec/public/.eslintrc.json)