Skip to content
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.

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

Js mixins, path mappings are applied even if modules are not used but are enabled #15967

Open
speedupmate opened this issue Jun 8, 2018 · 20 comments
Labels
Component: Other feature request Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. Progress: ready for grooming Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it

Comments

@speedupmate
Copy link
Contributor

speedupmate commented Jun 8, 2018

Js mixins and/or path mappings are applied even if modules are not used but are enabled. For example Magento 2 ships with Temando_Shipping extension , this defines mappings on few js files.

If Temando_Shipping is not configured from admin to be used on frontend , mappings still apply and affect the original behaviour that is needed when extension is not configured to be used.

Of course Temando_Shipping is just an example here , same applies to every mixin/mapping defined for any included extension in requirejs-config.js

Preconditions

  1. All Magento 2 versions

Steps to reproduce

  1. Install Magento with demo data 2.2.4 (don't configure Temando_Shipping)
  2. Add stuff to cart , reach checkout
  3. Temando_Shipping module mappings are active in checkout even if module is not enabled (not even arguing what issues it creates, but just to illustrate)

Expected result

  1. If extension is not configured to be used, we expect that it does not affect whatever is target for a mixin cause there is no need for customisation
  2. There needs to be a way to tie mixin enabling to configuration status not just enabled/disabled status of whole module. There might even be a need for more precise control on when mixins/mappings are applied by the area extension customises (by layout handler etc)

Actual result

  1. Temando_Shipping module mappings are active in checkout even if shipping method is not enabled from configuration.
  2. Dependant on developer skill or amount of methods altered this affects other extensions or even default modules to work as expected
@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jun 8, 2018
@speedupmate speedupmate changed the title Js mixins are applied even if modules are not used but are enabled Js mixins, path mappings are applied even if modules are not used but are enabled Jun 8, 2018
@DanielRuf
Copy link
Contributor

The JS files are also always loaded.

@ghost ghost self-assigned this Jul 27, 2018
@ghost ghost added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Component: Other labels Jul 27, 2018
@ghost
Copy link

ghost commented Jul 27, 2018

Hi @speedupmate thank you for your report.
We've acknowledged the issue and added to our backlog.

@ghost ghost removed their assignment Jul 27, 2018
@DrewML
Copy link

DrewML commented Dec 18, 2018

Possibly fixed by #19751

@speedupmate
Copy link
Contributor Author

No its not fixed by this as the issue is that module is enabled in sytem but not configured from backend to be on, enabled, configured to work. Example as Temando_Shipping is a shipping method and is only meant to be included in code when module is enabled and configured to be "on" from shipping methods.

To fix this Magento either needs 2 states of declaring a module enabled:

  1. enabled in system as bin/magento module:enable Some_Module
  2. enabled in system and enabled to work/configured from backend as a flag in config

or

Each mixin, js mapping needs to check if its module is enable before applying their changes

@magento-engcom-team magento-engcom-team added this to Ready for Dev in Community Backlog Mar 24, 2020
@sidolov sidolov added this to Ready for Grooming in Low Priority Backlog Sep 3, 2020
@sidolov sidolov added this to Ready for Development in Low Priority Backlog Sep 24, 2020
@ghost ghost removed this from Ready for Dev in Community Backlog Oct 20, 2020
@ghost ghost removed this from Ready for Development in Low Priority Backlog Oct 20, 2020
@ghost ghost removed Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Oct 20, 2020
@sivaschenko
Copy link
Member

sivaschenko commented Nov 9, 2020

@speedupmate I would expect that all mixins introduced by an enabled module are active.

If the mixin functionality depends on certain configuration settings (i.e enabled) the implementation should include the corresponding condition.

Please let me know what you think about that.

@speedupmate
Copy link
Contributor Author

@sivaschenko and how do you see this being implemented on what level and how exactly?

You can trick requirejs-config.js by writing js conditions inside it as it is a js file after all. For example reading some state from window.context and true/false your mixin status.

However if this would be preferred way it should be documented and included by default. Then all extensions implementing mixins with "configuration level settings" should output their status to window scope or implement a feature detection some sort to make the decision when mixins are applied.

@magento-engcom-team magento-engcom-team added the Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. label Nov 30, 2020
@magento-engcom-team magento-engcom-team added the Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. label Nov 30, 2020
@m2-community-project m2-community-project bot added this to Ready for Development in Low Priority Backlog Nov 30, 2020
@stale
Copy link

stale bot commented Feb 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

@stale stale bot added the stale issue label Feb 15, 2021
@speedupmate
Copy link
Contributor Author

haha , you mean let it rot until it fixes itself ?

@stale stale bot removed the stale issue label Feb 15, 2021
@stale
Copy link

stale bot commented May 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

@stale stale bot added the stale issue label May 2, 2021
@speedupmate
Copy link
Contributor Author

haha , you mean let it rot until it fixes itself ?

@atIOCrON
Copy link

Any update on this? A deleted user said it was added to the backlog in 2018. What's going on with this in 2021?

@speedupmate
Copy link
Contributor Author

@sivaschenko this issue reported from 2018 bites hard today and 2.4.3 is extra plagued with this , see last referenced issues .

If there was a way to validate mixins against config values or coded conditions like layout nodes can would be great . Even if mistakes are continued to be made there would be a way for others to suggest or code and PR a proper solution for customers without bashing the core or 3rd party bundled closed source extensions.

For example reCaptcha is separated to lot of different modules, hardcoded cross dependencies between each other in mixin level, one mixins jquery methods (for all others in site) etc , is not enabled by default but still affects everything by default.

If all mixins are applied when module is present it tweaks the reality, defaults to unexpected state. Referenced issues show for the worst that it does so to the extent where it's just not usable out of the box without understanding why those issues happen, from where those originate and what to disable or force configure and use to avoid those.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 30, 2021

Hi @speedupmate,
Thank you so much for your report!

My thoughts on why this issue still wasn't implemented are that it's not easy to fix.

First of all, currently, we could deploy static content on the "build system" (more info here: https://devdocs.magento.com/guides/v2.4/config-guide/deployment/). In this case, the build system doesn't have a DB connection.

Secondly, after changing store configuration, some functionality might work incorrectly after changing some configuration in the admin. Enabling the mixin is a pretty significant change - there might be cases like broken add to cart / broken checkout / etc. I'm pretty that having issues while installing a new module is better than after enabling it. You will initially see these issues and decide to fix or won't deploy that module to production.

Third, needed to re-deploy static content (see as "do another deployment") when you're changing some configuration in the admin. I'm sure it's not an expected thing.

I just listed all that above and think implementing this feature will cause more issues than benefits, and I believe that we shouldn't implement this feature at all.

What do you think?

@speedupmate
Copy link
Contributor Author

speedupmate commented Nov 30, 2021

@ihor-sviziev I'm thinking that this should be implemented, if config is changed then change is expected. If this is also documented it will benefit more than harm as you can find the cause by reading documentation.

If module is not configured or is disabled by default in config level it should not be enabled on frontend and included in builds.

If this can't be done on build time then mixin functionality could be extended in frontend and before applying a mixin it can seek for some optional config value . This would be a pseudo enable/disable since its still built and bundled but not applied if config does not follow. It will be still better than nothing since bundled extensions would not wrap when config is not met. Simply put there would be an option to avoid blindly overwriting defaults.

@ihor-sviziev
Copy link
Contributor

@speedupmate, could you contact me in engcom slack to continue the discussion? Thank you!

@ihor-sviziev
Copy link
Contributor

As discussed with @speedupmate in Slack - such a feature will be handy for many cases when some extension starts failing some JS.
Good example - the bug in the bundled extension like #33741 (when the extension is disabled).

Together we found a pretty simple solution:

  1. Add "enabledFlags" to global requires config (or if it's not possible - create a new variable)
    var require = {
    \'baseUrl\': \'' . /* @noEscape */ $block->escapeJs($block->getViewFileUrl('/')) . '\'
    };';
var require = {
    'baseUrl': 'https://example.com',
    'enabledFlags': ['Some_Extension', 'Magento_CheckoutAgreements',...]
};
  1. Modify the mixins logic to support the following logic to support the config like this:
 config: {
        mixins: {
            'Magento_Checkout/js/action/place-order': {
                'Magento_CheckoutAgreements/js/model/place-order-mixin': {'enabled': true, 'ifconfig': 'Magento_CheckoutAgreements'}
            },
        }
}

getMixins: function (path) {
var config = module.config() || {},
mixins;
// Fix for when urlArgs is set.
if (path.indexOf('?') !== -1) {
path = path.substring(0, path.indexOf('?'));
}
mixins = config[path] || {};
return Object.keys(mixins).filter(function (mixin) {
return mixins[mixin] !== false;
});
},

  1. Add mechanism for adding "enabled flags" using "ifconfig" (or something similar) through extensions

@speedupmate
Copy link
Contributor Author

@ihor-sviziev thanks for thinking along and understanding the issue and offering simplest ways to solve this

@m2-community-project m2-community-project bot added this to Ready for Grooming in Feature Requests Backlog Dec 2, 2021
@m2-community-project m2-community-project bot removed this from Ready for Development in Low Priority Backlog Dec 2, 2021
@ihor-sviziev ihor-sviziev added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Dec 2, 2021
@ihor-sviziev
Copy link
Contributor

FYI I changed the type to a feature request related to dev experience, as it describes a new feature that will simplify debugging mixins.

@PascalBrouwers
Copy link
Contributor

No don't, they move all feature requests to the forum where they die.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 2, 2021

@PascalBrouwers, so far now the feature requests are not moving to forums.
You can see them there: https://github.com/magento/magento2/projects/30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Other feature request Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. Progress: ready for grooming Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Feature Requests Backlog
  
Ready for Grooming
Development

No branches or pull requests

9 participants