Financial-Times / o-ads

Deprecated see README
http://registry.origami.ft.com/components/o-ads
14 stars 9 forks source link

Refactor modules #228

Closed alexflorisca closed 4 years ago

alexflorisca commented 5 years ago

There are a few issues that have been causing a lot of yak shaving work recently which need to be addressed.

Strong coupling o-ads uses a singleton pattern, loading all of its modules onto the main Ads singleton. This is then reinserted back into each module, therefore each modules, has indirect access to every other module via this.instance or this.ads. This is silly and it means a lot of the unit tests are more complicate than they need to be.

For example, in main.js we have:

import api from './src/js/data-providers/api';
Ads.prototype.api = api;

then in api.js we have:

Api.prototype.init = function(config, oAds) {
    this.config = config;
    this.instance = oAds;
        ...

This happens in almost all modules.

As a solution, we could make each module self contained, and import only what we need. For example, if we need to access the krux module from the api module, it would make sense to just import krux from./krux`. This can then be stubbed out in unit tests, making it clear what's happening.

State We have data being stored in different modules. The Api module stores targeting data. The Slots module has all the slots on the page. Etc, etc. This is one of the reasons I assume o-ads was designed the way it was, passing this.instance for access to all the modules.

We could also put all the state for the app in one place. Like Redux does. This could then be imported and accessed from every module, can be mocked in tests, etc.

Prototypes Another problem is the use of prototypes. We are relying on the use of this.xxx a lot throughout our code, which is hard to follow. Instead we could use object composition instead. This is great article to read on this design pattern:

https://medium.com/javascript-scene/master-the-javascript-interview-what-s-the-difference-between-class-prototypal-inheritance-e4cd0a7562e9

alexflorisca commented 4 years ago

Closing this as not actively developing o-ads anymore