0015 - Split business logic from dom logic of JavaScript classes and components

[NeOMakinG]

Here is the proof of concept link : https://github.com/PrestaShop/PrestaShop/pull/25909.

[NeOMakinG]

I agree with this, although we still have to decide where we will store this code, this should be decided (and documented) before validating the ADR no?

Agree

[eternoendless]

I agree on the separation of concerns, but I'm not so sure about the helpers. I'm worried that we end up with hundreds of lone functions that don't respect anything from functional programming and end up taking us back to imperative, pre-object-oriented programming.

Can we separate the two subjects so that we can move on with the former while we discuss on the latter?

[NeOMakinG]

I agree on the separation of concerns, but I'm not so sure about the helpers. I'm worried that we end up with hundreds of lone functions that don't respect anything from functional programming and end up taking us back to imperative, pre-object-oriented programming.

Can we separate the two subjects so that we can move on with the former while we discuss on the latter?

I really really think that these two subjects shouldn't be split:

If we choose to split concerns, we need a solution to do it properly in a maintainable way, and if we decide without the solution we will end up with some classes linked to the feature we are working on which will never be reused and might even be written again because you don't know it exists inside a specific feature's folder. Merging this ADR, without deciding on the way we should implement this idea will lead to confusion: "I should start this feature, but I know we decided to split business logic from others logic, how should I do? Wait, we use class everywhere, let's do a simple class which will deal with this logic". It's not following today's best practices, libraries logic and may cause performance issues on unit test run and usability of our features.

[eternoendless]

My initial request was to stop mixing up DOM updates with business logic. It doesn't mean we need to create a generic and reusable way to do it – heck, a lot of frameworks like React, and Vue have already solved this problem, but we can't really use them right now for reasons you already know.

This is an example of what I'm trying to avoid:

class MySetting {
    enabled: bool = false;

    public enable(): void {
        this.enabled = true;                      // <-- business logic
        $('#fooCheckbox').prop('checked', true);  // <-- view update
    }
}

new MySetting().enable();

In the aforementioned frameworks, this is solved by data binding. You just update bind mysetting.enabled to the checkbox's checked property, and it reacts to the change.

Without using a framework that supports data binding, we need to perform state management ourselves and find out the best way to notify views when state changes, and vice versa. The view always knows the store, but the idea is to try and make it so that the store knows as little as possible about the view.

I see three possible solutions:

1) Custom lightweight separation

The first approach consists of separating the business handling object from a ViewController object, but keeping them tightly coupled:

class MySetting {
    enabled: bool = false;
    view: MySettingView;

    constructor(view: MySettingView) {
        this.view = view;
    }

    public enable(): void {
        this.enabled = true;
        this.view.onEnable();
    }
}

class MySettingView {
    public onEnable(): void {
        $('#fooCheckbox').prop('checked', true);
    }
}

new MySetting(new MySettingView()).enable();

Advantages: dead simple, provides a little more context on what's happening, allows MySettings to be unit tested (by mocking MySettingView).

Disadvantages: the storage is still tightly coupled to the view.

2) Custom store component that implements the observer

The second approach builds on the previous one, but the difference is that instead of coupling the MySetting to MySettingView, MySettingView subscribes to changes in MySetting, then reacts accordingly.

// an example an observer implementation, blanks to be filled out
class Store {
    public subscribe(observer: object, prop: string, callback: function): void { /*...*/ }
    protected notify(changes): void { /*...*/ }
}

class MySetting extends Store {
    enabled: bool = false;

    public enable(): void {
        this.enabled = true;
        // [do some magic to publish the state change here]
    }
}

class MySettingView {
    construct(MySetting: store) {
        store.subscribe(store, 'enabled', this.onEnable.bind(this));
    }

    public onEnable(): void {
        $('#fooCheckbox').prop('checked', true);
    }
}

const mySetting = new MySetting();
const mySettingView = new MySettingView(mySetting))
mySetting.enable();

Another possible implementation would be using a pub/sub pattern, where the store sends out events through an event manager, and the view subscribes to them. It's essentially the same.

Advantages: the storage doesn't know about the view (it just notifies subscribers of data changes), it's relatively easy to implement.

Disadvantages: we go deeper into wheel reinventing territory, the actual working implementation will be probably more convoluted than the example above (because you need to keep track state of each individual property and its subscribers).

3) Adopt an existing state management framework

There are existing frameworks that have this figured out, like Vuex, Redux and MobX.

I've researched the three a bit, and here are my conclusions for each one:

Vuex is made to work with Vuejs, which is good for our future-proofing. I and according to some it could be made to work on non-Vuejs components, the tricky part still being how to notify our view controllers of the store changes. According to what I read, it would looks somewhat like this:

// the store
const mySetting = new Vuex.Store({
    state: {enabled: false},
    mutations: {
        enable(state, data) {
            state.enabled = true;
        }
    },
    getters: {
        isEnabled: state => state.enabled
    }
}

class MySettingView {
    construct() {
        store.watch(state => state.enabled, (newValue, oldValue) => this.onEnable());
    }

    public onEnable() {
        $('#fooCheckbox').prop('checked', true);
    }
}

const mySettingView = new MySettingView();
mySetting.commit('enable');

I had a look at Redux, but it looks like it's more complex and heavily geared towards React. I also found its terminology a little obscure (reducers, slices, ...), so I felt like it could become too complicated really quick for a nonstandard use case like ours.

Finally, I spent a bit of time with MobX, which looks more unopinionated than the other two, and I found a working example with Jquery, so we know it's possible (even if the example looks old).

It would look like this:

import { makeAutoObservable, reaction } from "mobx";

class MySetting {
    enabled: bool = false;

    constructor() {
        makeAutoObservable(this);
    }

    public enable(): void {
        this.enabled = true;
    }
}

class MySettingView {
    construct(MySetting: setting) {
        // this is triggered when the "enabled" property is changed
        reaction(() => setting.enabled, (isEnabled) => this.onEnable());
    }

    public onEnable() {
        $('#fooCheckbox').prop('checked', true);
    }
}

const mySetting = new MySetting();
const mySettingView = new MySettingView(mySetting))
mySetting.enable();

Advantages: Not reinventing the wheel Disadvantages: We might get stuck in difficult use cases, each library has its tradeoffs, the library we choose might influence our future tech choices

---

I think everyone will agree that option (2) is not a good idea. We don't want to continue reinventing the wheel.

Option 3 is the most interesting one for future-proofing, but it could also prove to be a time black hole. From the looks of it, MobX's API looks really simple, so I think it's worth a shot to investigate it a little further and find out if it's feasible to use it as our state management tool.

If it doesn't work out or if we don't find the time to do test that out, I would go with option (1) for the time being – it has the best cost/benefit ratio given the current state of our scripts.

[NeOMakinG]

@eternoendless

Option 1 looks good to me, because we don't rely on any external libraries that we will be stuck with in the future, but I'm really afraid that we will duplicate a lot of logic between every page. Price calculation can be done on multiple pages (Order + Product page for example), and so we will import the full product page price calculation component on the order page in order to achieve some maths... Should we mix this state/view pattern with some helpers?

Option 2 is definitely wrong, Vue, React, Angular, Svelte, and a lot more libraries maintain this "pattern" for years and we know Vue will take more and more place in our project, let's not reinvent something already existing.

Option 3 => VueX is a store, a store is designed to extract shared states between multiple components, it's not designed to split business logic from the view, Vue does it by its own, this choice is valuable for a page such as the product one, but it's not the case on every page, and most of our pages doesn't have shared states, to be honest. Using a store to contain every state of our application, regardless of the fact that the state is not shared between multiple components, really makes it harder to maintain a project.

MobX is a good approach because it's not firstly designed to be a store, at first it's a state management system not linked to any UI, which means that it's fully out of the box and can be used with any library. But I'm not a big fan of this solution because it will lead to a complete miss-use in the future => If we use Vue everywhere, we will have EVERY state inside MobX, while Vue itself could own maybe 90% of the states we need instead of a store (because remember, a store is designed to contain shared states), so it will lead to a complete refactor on Vue migration, same as if we choose the solution 1 anyway...

If we choose to add some helpers, regardless of the class implementation of any pages, we will be able to use them as Vue mixins in the future or even by just importing them.

[PierreRambaud]

heck, a lot of frameworks like React, and Vue have already solved this problem, but we can't really use them right now for reasons you already know.

So, the best way is to switch to React or Vue, not redo the wheel. :sweat_smile:

With your propositions, we are going to increase the front complexity, when we don't know where we are going with the front in a back-office context.

About option 1, I agree with @NeOMakinG, I don't think it's a good idea, it's not reusable easily when the original ADR is here to add helpers in order to be able to create units test because right now, there is nothing (except CLDR component) I also think this ADR can be separated from what you want @eternoendless

[eternoendless]

Helpers don't solve the original problem. My original request was to separate business logic (state management) from view management (DOM manipulation). I never said I wanted to extract helpers to reuse logic, I'm not fundamentally against it, but it's a completely different subject.

[PierreRambaud]

Helpers don't solve the original problem. My original request was to separate business logic (state management) from view management (DOM manipulation). I never said I wanted to extract helpers to reuse logic, I'm not fundamentally against it, but it's a completely different subject.

So, the real solution is to use Vue or React, not redo the wheel. Adding a new dependency for doing the same thing as Vue already does is really a bad idea. I totally agree with what @NeOMakinG said above :/

[eternoendless]

Then let's go with option 1

Using Vue is not really an option as long as we don't figure out how to

1. Build Vue components using Twig in the backend
2. Make it extensible