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

[NeOMakinG]

Here is the proof of concept link : PrestaShop/PrestaShop#25909.

[jolelievre]

TLDR; I lie the idea but it seems to be only applicable on very small pieces of code Maybe my vision is flawed because your example is simple for now

Tat said I also think testing could be applied on a wider domain like the whole product model (see comments inside the PR)

[jolelievre]

I like the idea but I'm afraid it only tackles with simple use case here, and the complexity is not about computing tax ratio, or apply tax to tax included fields.

The complexity is raised when you are dealing with inter-connected fields that influence each other and you need to be sure that modifying one will correctly update the others.

And I don't think this can be handled easily with functional programming.

[jolelievre]

That said having small functions for small actions (like tax ratio) that are tested is a good idea, the question being where should they be stored, and what domain do they actually belong to?

[jolelievre]

Finally, I don't completely agree with what you were saying about the impossibility of testing with a DOM, especially regarding the new product page. But you're unlucky you chose, maybe, the only page where it's not completely accurate 😅

Because the ProductFormModel relies on the ObjectFormMapper and even if it is true that its main purpose is to rely on a form and maintain its synchronization it can also handle the model purely internally. You can get/set any field of the configured model, the event will be triggered and the appropriate actions will also trigger.

So the whole product model along with its rules should be testable in a pure JS way I think.

I admit some parts may be missing:

• the ability to inject an initial full model into the ObjectFormMapper because for now its initialization is only based on form reading
• make sure that the absence of form won't cause a bug, which I can't be 100% sure since it's never been tested this way, but it is definitely doable

[NeOMakinG]

The complexity is raised when you are dealing with inter-connected fields that influence each other and you need to be sure that modifying one will correctly update the others.

And I don't think this can be handled easily with functional programming.

I really think that testing multiple inter-connected fields is not unit tests role and more e2e, to my opinion, unit tests are designed to test simple I/O functions without a lot of dependencies

[NeOMakinG]

That said having small functions for small actions (like tax ratio) that are tested is a good idea, the question being where should they be stored, and what domain do they actually belong to?

This is something we need to reply all together:

• Inside the component folder using a subfolder?
• Inside a global folder?
• Should we upload these components into an npm module for the community/module usage?

[NeOMakinG]

I admit some parts may be missing:

• the ability to inject an initial full model into the ObjectFormMapper because for now its initialization is only based on form reading
• make sure that the absence of form won't cause a bug, which I can't be 100% sure since it's never been tested this way, but it is definitely doable

This means that you develop a functionality only to be able to test the behavior, it looks unnatural as if you follow a development workflow, it's naturally testable

[jolelievre]

I really think that testing multiple inter-connected fields is not unit tests role and more e2e, to my opinion, unit tests are designed to test simple I/O functions without a lot of dependencies

Maybe for the FormObjectMapper it would not be strictly unit test and rather integration tests, especially since it integrates the EventEmitter but this could also be mocked So unit tests is not totally inconceivable for this component especially if it becomes independent from the form

Even if it needs a DOM, it could be possible to test it with dedicated HTML files, but anyway that's not the topic of this ADR anyway ^^

This means that you develop a functionality only to be able to test the behavior, it looks unnatural as if you follow a development workflow, it's naturally testable

It's true that the component was initially thought to work on forms only, but it turns out it also handles some pure model feature, it keeps a central source of knowledge of the model Allows to get/set some of its parts, handle events to notify its updates All these features could be independent from the form, the initial data could be filled via an API for example TLDR; I don't want to change the behaviour purely to allow testing but also it could be a nice feature

[NeOMakinG]

But a dedicated HTML file is really hard to maintain (because you can have a delta between the templating system and this HTML file and I hardly think that we shouldn't do this because when you use the DOM, it's probably not linked to unit tests at all but E2E

[matks]

I think that, overall, this is a good idea.

If we think about, we're simply applying the MVC principle to our JavaScript logic 😄

Today single Javascript files

• fetch data from the DOM (Controller responsibility)
• modify this data using business logic (Model responsibility)
• update the DOM following the new DOM state (View responsibility)

Right here we're separating the Model out of the View and Controller.

The only thing I would be worried about is the amount of boilerplate code sometimes needed.

In your POC, the result code looks like this

const taxRatio = calculateTax($selectedTaxOption.data('taxRate'));

One input, one function, one result cool.

But if tomorrow we build a Specific Price Validator in JS that needs

• product data
• price data
• customer data (!)
• customer group data (!)
• moar data

Then the code might look like

const isValid = validateSpecificPrice(
    $productContainer.data('type'),
    $productContainer.data('has_combinations'),
    $productContainer.data('unit_price'),
    $priceDiv.data('value'),
    $customerBlock.data('customer-id'),
    $customerGroupBlock.data('...'),
);

In such a usecase, the developer writing the above logic will do multiple back-and-forth between the component and the file.

But the fact it's hard comes from the complexity of the usecase. Splitting is always good.

So GG for me

[jolelievre]

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?

[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. 😅

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