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

x-magento-init initialisation not bound to happen in the right order. #17125

Closed
kshaa opened this issue Jul 25, 2018 · 7 comments
Closed

x-magento-init initialisation not bound to happen in the right order. #17125

kshaa opened this issue Jul 25, 2018 · 7 comments
Assignees
Labels
Area: Frontend Component: Theme Fixed in 2.4.x The issue has been fixed in 2.4-develop branch 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 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 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

Comments

@kshaa
Copy link

kshaa commented Jul 25, 2018

Preconditions

  1. Magento 2.1.12

Steps to reproduce

  1. Inject a script very early in the page, that requires section-config.js and trigger its method getAffectedSections("some page").

Example reproduction code
(Or just use this example module - https://github.com/kshaa/m2-section-config-oddity/)

vendor/magento/module-theme/view/base/templates/root.phtml
(Root.phtml is not the proper place for customization, this is just an example for the issue)

<?php
/**
 * Copyright © 2013-2017 Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

// @codingStandardsIgnoreFile

?>
<!doctype html>
<html <?php /* @escapeNotVerified */ echo $htmlAttributes ?>>
    <head <?php /* @escapeNotVerified */ echo $headAttributes ?>>
        <?php /* @escapeNotVerified */ echo $requireJs ?>
        <?php /* @escapeNotVerified */ echo $headContent ?>
        <?php /* @escapeNotVerified */ echo $headAdditional ?>
    </head>
    <body data-container="body" data-mage-init='{"loaderAjax": {}, "loader": { "icon": "<?php /* @escapeNotVerified */ echo $loaderIcon; ?>"}}' <?php /* @escapeNotVerified */ echo $bodyAttributes ?>>
        <script>
            require([
                "Vendor_Theme/js/test"
            ], function(test) {
                var affectedSections = test("some page");
            });
        </script>
        <?php /* @escapeNotVerified */ echo $layoutContent ?>
    </body>
</html>

app/code/Vendor/Theme/view/frontend/web/js/test.js

define([
    'jquery', // Because everything requires jQuery
    'Magento_Customer/js/section-config'
], function($, sectionConfig) {

    return function(page) {
        sectionConfig.getAffectedSections(page);
    }
});

Expected result

  1. RequireJS will load section-config.js
  2. section-config.js constructs and initiates itself as required
  3. My injected script receives all affected sections for the URL "some page"

Actual result

  1. RequireJS loads section-config.js, but the internal data section-config requires for functioning isn't loaded yet
  2. section-config.js throws an error: "Uncaught TypeError: Cannot read property '*' of undefined"

Extra information

How I expect this internal section-config data load works usually

I think the data for section-config usually gets initiated in the following way:
vendor/magento/module-customer/view/frontend/templates/js/section-config.phtml template embeds the section-config with an x-magento-init script and also provides the required internal data
lib/web/mage/apply/scripts.js triggers early on page-load and transforms that x-magento-init into a virtual component (JS variable to-be-processed)
lib/web/mage/apply/main.js loads that section-config virtual component and binds the data from the template by passing it to a function that corresponds to the section-config returned object key "Magento_Customer/js/section-config".

Questions and concerns

  1. That "usual" data loading approach only works if section-config gets first loaded by the x-magento-init mechanism, however it's not given that will always be the case.
  2. Is it documented somewhere that I'm not allowed to use a bare-bones 'require' function for receiving section-config and other base features?
  3. I didn't manage to reproduce the following idea, but - if I x-magento-init'ed that test.js and once again required section-config, then it should also fail right? Because main.js (the x-magento-init JS mechanism) wouldn't have loaded the internal section-config data, because it simply uses a bare-bones require without any safety-checks.

Am I missing something critical here? Does the x-magento-init mechanism have drawbacks/limitations that I haven't seen in the documentations?
I just want to understand how this works, maybe this is actually easy and I didn't notice something.

P.S. I found this issue, because somewhere early in my page is a bare-bones JS using require that calls section-config within it's long dependency tree and fails in the aforementioned way and I can't easily fix it by replacing that initial script with x-magento-init or by moving it lower in the page, because I don't know what initial script triggers this faulty dependency tree.
P.P.S. This also seems like a race-condition which depends on the time it takes to load JS libraries over the network. Sometimes there is an error, sometimes not, but that shouldn't happen.

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jul 25, 2018
@magento-engcom-team
Copy link
Contributor

Hi @kshaa. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me {$VERSION} instance

where {$VERSION} is version tags (starting from 2.2.0+) or develop branches (2.2-develop +).
For more details, please, review the Magento Contributor Assistant documentation.

@kshaa do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@kshaa
Copy link
Author

kshaa commented Jul 25, 2018

Hey, @magento-engcom-team,

☑Yes, I successfully reproduced this issue also on a vanilla Magento 2.2.5.
Here's the example code I used - https://github.com/kshaa/m2-section-config-oddity/

@engcom-backlog-nickolas engcom-backlog-nickolas added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development 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 Area: Frontend labels Jul 26, 2018
@engcom-backlog-nickolas engcom-backlog-nickolas removed their assignment Jul 26, 2018
@engcom-backlog-nickolas

Hello @kshaa, thank you for your report.
We've acknowledged the issue and added to our backlog.

@kshaa
Copy link
Author

kshaa commented Sep 5, 2018

In the project where this issue appeared it seemed like it was caused because of such a flow:

  1. customer-data.js initialises and requires section-config.js
  2. section-config.js loads (by require), but still doesn't have its internal data loaded (from vendor/magento/module-customer/view/frontend/templates/js/section-config.phtml)
  3. Some 3rd party script or anything really fires a POST request
  4. customer-data notices the POST request and triggers section invalidation
  5. section-config.js fails with an exception "Uncaught TypeError: Cannot read property '*' of undefined", because it's internal data (more precisely the section invalidation list "sections") hasn't loaded

I fixed it by implementing jQuery "promises" in the section-config and then using that promise in the customer-data invalidation functionality.
Essentially that fixes it by making the flow as such:
4. customer-data notices the POST request and uses a section-config promise (of when internal data will be ready) to lazily trigger an invalidation
5. later section-config internal data loads and it all promises are resolved
6. success - no error

vendor/magento/module-customer/view/frontend/web/js/section-config.js
From:

        'Magento_Customer/js/section-config': function(options) {
            baseUrls = options.baseUrls;
            sections = options.sections;
            clientSideSections = options.clientSideSections;
        }

To:

        /**
         * Promises of affected sections
         */
        sectionPromises: [],

        /**
         * Return and store promise of affected sections
         * or return actual affected sections
         *
         * @param url
         * @returns {*}
         */
        getAffectedSectionsPromise: function(url) {
            var deferred = $.Deferred();

            // If sections are already loaded, don't promise, just return
            if (typeof(sections) !== 'undefined') {
                return this.getAffectedSections(url);
            }

            this.sectionPromises.push({
                promise: deferred,
                url: url
            });

            return deferred.promise();
        },

        /**
         * Resolve all stored affected section promises
         *
         * Note: Should be run internally, when all section data is loaded
         */
        resolvePromises: function() {
            var self = this;

            this.sectionPromises.forEach(function(bound) {
                var affectedSections = self.getAffectedSections(bound.url)

                bound.promise.resolve(affectedSections);
            });

            this.sectionPromises = [];
        },

        'Magento_Customer/js/section-config': function(options) {
            baseUrls = options.baseUrls;
            sections = options.sections;
            clientSideSections = options.clientSideSections;

            this.resolvePromises();
        }

vendor/magento/module-customer/view/frontend/web/js/customer-data.js
From:

    /**
     * Events listener
     */
    $(document).on('ajaxComplete', function (event, xhr, settings) {
        var sections,
            redirects;

        if (settings.type.match(/post|put|delete/i)) {
            sections = sectionConfig.getAffectedSections(settings.url);

            if (sections) {
                customerData.invalidate(sections);
                redirects = ['redirect', 'backUrl'];

                if (_.isObject(xhr.responseJSON) && !_.isEmpty(_.pick(xhr.responseJSON, redirects))) {
                    return;
                }
                customerData.reload(sections, true);
            }
        }
    });

    /**
     * Events listener
     */
    $(document).on('submit', function (event) {
        var sections;

        if (event.target.method.match(/post|put|delete/i)) {
            sections = sectionConfig.getAffectedSections(event.target.action);

            if (sections) {
                customerData.invalidate(sections);
            }
        }
    });

To:

    /**
     * Events listener on network requests to conditionally trigger section invalidation
     *
     * Overridden to properly handle sectionConfig not yet having internal data initialised
     *
     * Reference - https://github.com/magento/magento2/issues/17125
     */
    $(document).on('ajaxComplete', function (event, xhr, settings) {
        var redirects;

        if (settings.type.match(/post|put|delete/i)) {
            // Wait for customerData internal readiness
            $.when(customerData.getCustomerDataPromise())
                .done(function() {
                    // Wait for affected sections internal readiness
                    $.when(sectionConfig.getAffectedSectionsPromise(settings.url))
                        .done(function(sections) {
                            // Trigger section invalidation
                            if (sections) {
                                customerData.invalidate(sections);
                                redirects = ['redirect', 'backUrl'];

                                if (_.isObject(xhr.responseJSON) && !_.isEmpty(_.pick(xhr.responseJSON, redirects))) {
                                    return;
                                }

                                customerData.reload(sections, true);
                            }
                        });
                });
        }
    });

    /**
     * Events listener on form submissions to conditionally trigger section invalidation
     *
     * Overridden to properly handle sectionConfig not yet having internal data initialised
     *
     * Reference - https://github.com/magento/magento2/issues/17125
     */
    $(document).on('submit', function (event) {
        if (event.target.method.match(/post|put|delete/i)) {
            // Wait for customerData internal readiness
            $.when(customerData.getCustomerDataPromise())
                .done(function() {
                    // Wait for affected sections internal readiness
                    $.when(sectionConfig.getAffectedSectionsPromise(event.target.action))
                        .done(function (sections) {
                            if (sections) {
                                // Trigger section invalidation
                                customerData.invalidate(sections);
                            }
                        });
                });
        }
    });

P.S. The promise implementation could be written in a shorter way - certainly.
P.P.S. This fix only fixes a small part of the whole problem. Using x-magento-init for getting data into components is a faulty idea, IMO, because initially the component won't have data - this isn't documented anywhere (I may be wrong). This fix just makes section-config and customer-data work well in conjunction while also taking into account that the required internal section-config data won't immediately be available.

@DrewML
Copy link

DrewML commented Aug 22, 2019

Looks like there is a big temporal coupling problem here.

In section-config.js, it exports an API when the module is initialized, and it declares 4 variables (baseUrls, sections, clientSideSections, canonize) in the module body's closure.

However, the exported API is not actually in a usable state until the Magento_Customer/js/section-config method has been invoked (by a mage-init script, as mentioned) which then sets the values in that closure.

A similar bug with the same root cause was recently patched by @davidverholen in #23099.

@krzksz
Copy link
Contributor

krzksz commented Oct 4, 2019

Hey @kshaa!
You are right in your investigations and since we've recently hit the same problems that you have I spent some time to work on a solution which doesn't require too much API changes in Magento itself. Could you please check if #24847 is an applicable solutions for you? Once merged we will be able to backport it to older Magento versions then 2.3.x.

@ghost ghost assigned krzksz and unassigned krzksz Oct 29, 2019
@ghost ghost unassigned krzksz Nov 26, 2019
@ghost ghost assigned krzksz Nov 26, 2019
@slavvka
Copy link
Member

slavvka commented Feb 8, 2020

Hi @kshaa. Thank you for your report.
The issue has been fixed in #25764 by @krzksz in 2.4-develop branch
Related commit(s):

The fix will be available with the upcoming 2.4.0 release.

@slavvka slavvka added the Fixed in 2.4.x The issue has been fixed in 2.4-develop branch label Feb 8, 2020
@slavvka slavvka closed this as completed Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Component: Theme Fixed in 2.4.x The issue has been fixed in 2.4-develop branch 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 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 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
Projects
None yet
Development

No branches or pull requests

7 participants