Elasticsuite: Magento core JS not extended but replaced entirely (This is wrong)

[stephan-cream]

A core file of Magento_Swatches is replaced entirely through requirejs-config.js.
magento/module-swatches/view/adminhtml/web/js/product-attributes.js
-> elasticsuite/src/module-elasticsuite-swatches/view/adminhtml/web/js/product-attributes.js

This goes agains the principles of Magento 2, mainly due to the fact that when changes occur to Magento's core, it will stop working when the core is upgraded/patched.

The use of mixins to extend logic would be preferable.
https://devdocs.magento.com/guides/v2.2/javascript-dev-guide/javascript/js_mixins.html

All you would need is to make 1 mixin, and modify the functions like this:

 var mixin = {
    // There should be no need to rewrite the bindAttribute function, just this array
     selectFields: ['select', 'multiselect', 'price', 'swatch_text', 'swatch_visual', 'text', 'boolean'],

     // For the addition of texteditor textarea visibility
     switchDefaultValueField: function () {
         // Function logic as currently in your file
         // However i believe this could be done more efficiently like:
         this.super();

         var defaultValueTextareaVisibility = false;
         var currentValue = this.frontendInput.val();

         if (currentValue === 'texteditor') {
            defaultValueTextareaVisibility = true;

            if (optionConfig.hiddenFields[currentValue]) {
                $.each(optionConfig.hiddenFields[currentValue], function (key, option) {
                    switch (option) {
                        case '_front_fieldset':
                            thing.tabsFront.hide();
                            isFrontTabHidden = true;
                            break;

                        case '_default_value':
                            defaultValueTextVisibility = false;
                            defaultValueTextareaVisibility = false;
                            defaultValueDateVisibility = false;
                            defaultValueYesnoVisibility = false;
                            break;

                        case '_scope':
                            scopeVisibility = false;
                            break;
                        default:
                            thing.setRowVisibility($('#' + option), false);
                    }
                });

                if (!isFrontTabHidden) {
                    thing.tabsFront.show();
                }
            }

            this.setRowVisibility(this.defaultValueTextarea, defaultValueTextareaVisibility);
         }
     }
 };

 return function (parent) {
     return parent.extend(mixin);
 };

Module version: irrelevant
Magento Version : irrelevant
ElasticSuite Version : irrelevant
Environment : irrelevant
Third party modules : irrelevant

[stephan-cream]

As a sidenote, the reason i initially encountered this as a "problem" is because we patched 2.2.7 but this module broke functionality with saving attribute options on the module version 2.6.4 even though it was supposedly compatible with Magento 2.2.7.
Looking at the code differences between your file and Magento's, there seems to be no need to replace the entire file as the changes are so minimal.

[rbayet]

Hello @stephan-cream,

I agree with you wholeheartedly, the override on those two natives files are historic stones in our shoes (right and left feet).
For a while the pain was sustainable, but they have been numerous (non BC) changes in 2.2.x (for instance between 2.2.5, 2.2.6 and 2.2.7) for Magento to cover a range of issues.

We detected the changes in 2.3.x / 2.3.0, not the backport in 2.2.7.
The 2.6.6 release addresses this issue with a fix which covers from M2 CE 2.2.1 to 2.2.7 included (EE is covered as well, even if you cannot install it directly on top of an EE < 2.2.3 without composer trickery).

But we are aware we need to change our approach: looking at the magento bugtracker, there will be additional changes in those two files (specifically the module-elasticsuite-swatches one).

Your proposal for the use of a mixin is a step in the right direction.
I'll let @romainruaud voice his opinion, but feel free to submit a PR, we will gladly have a look at it.

Regards,

[romainruaud]

Hello @stephan-cream

to be completely transparent, I was not aware of how to use mixins and how they could be valuable to fix such things.

So first of all, thank you, it's also for things like this that I do enjoy writing Open Source software, because of other users teaching me some tricks to improve our code.

Using a mixin seems way more efficient and less intrusive than our previous approach. I think we should definitively move forward and adopt it.

Would you mind submitting a PR for this one ? I won't be upset if you do not take additional time on this one, since you already helped a lot by pointing us the proper way to do this.

Regards

[stephan-cream]

I will look at this later today to provide a PR.

[romainruaud]

any news @stephan-cream ?

[stephan-cream]

@romainruaud My apologies, it is quite busy here so i don't really have time to look into this.
However i am pretty sure the above mentioned code is quite close to the actual solution.
Perhaps look at
vendor/magento/module-catalog/view/adminhtml/web/js/components/use-parent-settings/toggle-disabled-mixin.js
and
vendor/magento/module-catalog/view/adminhtml/requirejs-config.js
for the example of an existing plugin.

[rbayet]

Hello @stephan-cream,

Unfortunately, the mixin approach you described will not work in this case, because vendor/magento/module-swatches/view/adminhtml/web/js/product-attributes.js returns a function which does not return anything and not an object (which would have the extend method).

The alternative mixin syntax/approach pointed by the docs and used in https://github.com/magento/magento2/blob/2.3/app/code/Magento/CheckoutAgreements/view/frontend/web/js/model/place-order-mixin.js cannot address the issue either.

The minimal need here is to alter the selectFields field/property of the swatchProductAttributes anynomous object which is simply defined and used in that function (and is not a class variable of returned object).

In a perfect world, selectFields would be defined by optionConfig.selectFields in vendor/magento/module-swatches/view/adminhtml/web/js/product-attributes.js and then, a mixin could be used to inject the additional supported frontend input types.

I might be wrong, but I'm pretty sure I'm not (sadly).

Regards,
Richard.

[romainruaud]

So maybe we could propose a PR to Magento in order to cleanup the mess on this area, and to allow third-party extension developers to extend it easily :)