Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
NeOMakinG wants to merge 3 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d196477
0015 - Split business logic from dom logic of JavaScript classes and …
Sep 16, 2021
2b353ab
Correct grammar
Sep 16, 2021
9419529
Update the ADR
Sep 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions 0015-split-business-logic-from-dom-logic.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# 15. Split business logic from dom logic of JavaScript classes and components

Date: 2021-09-16

## Status

In discussion

## Context

PrestaShop developers are working on JavaScript components by mixing some DOM handlers with some logic not linked to the DOM at all.

For example:
The new product page is calculating some prices depending on events and form changes, for this, it takes some data from the DOM and manipulates these data at the same time as updating the DOM.

It cause some issues:

- We can't use unit tests, because they rely on the DOM generated by a template engine (Twig, smarty...), except if we create a virtual DOM with hardcoded markup which is unmaintainable.
- It's really hard to review because we use intern dependencies that you should be aware of to review properly. If concerns are split properly, it's easier to understand.

Benefits:

- JavaScript unit tests are possible.
- Better readability and it is easier to review.
- Helpers are reusable in the future with any front-end libraries.

## Decision

[Please, take a look at this proof of concept](https://github.com/PrestaShop/PrestaShop/pull/25909), some tax ratios calculations are split into a helper folder, using functional programming (because it's easier to understand the data flow: You have some data going in, you have some data going out, which is not the case of class programming in general because it's a more complex paradigm). And I added a unit test example using the helper. This unit test was impossible to be written with the class method, without having a complex virtual dom system.
Copy link
Copy Markdown
Contributor

@jolelievre jolelievre Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jolelievre jolelievre Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@jolelievre jolelievre Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@NeOMakinG NeOMakinG Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@NeOMakinG NeOMakinG Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@NeOMakinG NeOMakinG Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@jolelievre jolelievre Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@NeOMakinG NeOMakinG Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


What should we do:
- Use a global folder containing helpers with a relative path, they are designed to be reusable and as we are speaking about functional programming, the helper should not modify any software state, it's a basic I/O function that take some params and returns some data.
- Use functional programming, because it's more performing and doesn't require object logic.
- Every helper requires a unit test to be written to accept a pull request.

## Consequences

We have to think about the logic of our JavaScript code while working on something and split it as proposed. We also need to write unit tests.