magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

fixes requireJS mixins racecondition with require calls in dom

Open AndresInSpace opened this issue 1 year ago • 42 comments

Description (*)

Fixes the race condition with DOM require calls parsing before require(['mixins'],{...}); replacing require function. Resolves mixins not applying. Ensures jquery-mixin from security https://github.com/magento/security-package/blob/develop/ReCaptchaWebapiUi/view/frontend/requirejs-config.js#L10 is mixed in before jquery called

How to Confirm You Have This Issue: add to custom block output in top of page of cart closest to body opening tag, visit empty cart page /checkout/cart/index context should be from wrapper (mixin applied) not from customer-data (mixin not applied)

<script>
    require(['jquery', 'Magento_Customer/js/customer-data'], function($, customerData) {
        'use strict';

        console.log("context from mixin?: %O", customerData.getExpiredSectionNames);
    });
</script>

Original Process: requirejs makeRequire calls nexTick (setTimeout) freeing thread to parse DOM requirejs/mixins.js loads and define call is made DOM require calls race condition - if calls happen require context is from requirejs/require.js and not mixins.js requirejs/mixins.js require fired and now instantiating and replacing require call context to mixins.js

Changes: Moves requirejs-config.js before mixins.js ensures configurations are loaded Remove context.nextTick wrap from makeRequire return localRequire function (this was freeing thread to DOM) ** Remove redundant intakeDefines(); Move require calls out of require-configs. Only one is for jquery no conflict. Remove from https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Theme/view/base/requirejs-config.js#L87 and move to end of require(['mixins], function(..) { ... {beforeClosingtag} }); ensures mixin replaces require function so jquery-mixin is properly applied

**checkLoaded() has all the setTimeout functionality we need https://github.com/magento/magento2/blob/9124539dd5d26ea31dcda8608071582d2bfbde1a/lib/web/requirejs/require.js#L717 See more information in my comment on the issue this PR should fix below.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes magento/magento2#33593
  2. Properly Fixes magento/magento2#31920
  3. Addresses #38274 (config output may need hoisting see my comment in ticket)
  4. Should also fix magento/baler#23

Manual testing scenarios (*)

See the above issue for steps to reproduce mixins not loading issue caused from this race condition additionally can

  1. increase nextTick setTimeout value in requirejs.js to a larger number,increment more if needed to replicate consistently
  2. reload page
  3. verify dom require calls are parsed before require(['mixins']

Questions or comments

Contribution checklist (*)

  • [x] Pull request has a meaningful description of its purpose
  • [x] All commits are accompanied by meaningful commit messages
  • [ ] All new or changed code is covered with unit/integration tests (if applicable)
  • [ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • [ ] All automated tests passed successfully (all builds are green)

AndresInSpace avatar Aug 23 '24 02:08 AndresInSpace

Hi @AndresInSpace. Thank you for your contribution! Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:
  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] avatar Aug 23 '24 02:08 m2-assistant[bot]

@magento give me test instance

On Thu, Aug 22, 2024, 10:16 PM m2-assistant[bot] @.***> wrote:

Hi @AndresInSpace https://github.com/AndresInSpace. Thank you for your contribution! Here are some useful tips on how you can test your changes using Magento test environment. Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here https://github.com/magento/magento2/wiki/Magento-Automated-Testing ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions https://developer.adobe.com/commerce/contributor/guides/code-contributions/ documentation. Join Magento Community Engineering Slack https://developer.adobe.com/open/magento/slack and ask your questions in #github https://magentocommeng.slack.com/messages/C7KB93M32 channel.

— Reply to this email directly, view it on GitHub https://github.com/magento/magento2/pull/39097#issuecomment-2306059135, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNQ47QTMLXF2NWF4BSYBDDZS2LQDAVCNFSM6AAAAABM7IWNX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBWGA2TSMJTGU . You are receiving this because you were mentioned.Message ID: @.***>

AndresInSpace avatar Aug 23 '24 02:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

Hi @AndresInSpace, here is your Magento Instance: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering Admin access: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering/admin_9a90 Login: 9dd942e3 Password: bf5d160ddea5

@magento give me 2.4-develop instance

AndresInSpace avatar Aug 23 '24 03:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

Hi @AndresInSpace, here is your Magento Instance: https://45243a10eb203fde7e8eedf63293cfe1.instances-prod.magento-community.engineering Admin access: https://45243a10eb203fde7e8eedf63293cfe1.instances-prod.magento-community.engineering/admin_56cc Login: c47b2027 Password: e1500fe5aee1

Appears resolved based on testing customer-data in a require statement added via home block so it is very close to opening body tag in order to trigger the error.

<script>
    require(['jquery', 'Magento_Customer/js/customer-data'], function($, customerData) {
        'use strict';

        console.log("context from mixin?: %O", customerData.getExpiredSectionNames);
    });
</script>

View console log F12 & Reload w cache disabled or ctrl+f5 hard refresh chrome on windows

Vanilla instance shows customerData.getExpiredSectionNames from customer-data.js not the mixin override vanilla-getExpiredSectionNames-not-from-mixin

PR Instance correctly shows customerData.getExpiredSectionNames from wrapper, as mixin extended it. vanilla-getExpiredSectionNames-from-mixin

AndresInSpace avatar Aug 23 '24 03:08 AndresInSpace

@magento run all tests

AndresInSpace avatar Aug 23 '24 06:08 AndresInSpace

@magento give me test instance

AndresInSpace avatar Aug 24 '24 20:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

Hi @AndresInSpace, here is your Magento Instance: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering Admin access: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering/admin_64bd Login: 7a75a37a Password: 9984b8486411

@magento give me test instance

AndresInSpace avatar Aug 28 '24 04:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

Hi @AndresInSpace, here is your Magento Instance: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering Admin access: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering/admin_13df Login: 4b195ca5 Password: e2320ead2d47

@magento give me 2.4-develop instance

AndresInSpace avatar Aug 28 '24 05:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

Hi @AndresInSpace, here is your Magento Instance: https://45243a10eb203fde7e8eedf63293cfe1.instances-prod.magento-community.engineering Admin access: https://45243a10eb203fde7e8eedf63293cfe1.instances-prod.magento-community.engineering/admin_a159 Login: 728298b6 Password: c368314a96b0

@magento give me 2.4-develop instance

AndresInSpace avatar Aug 28 '24 18:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

@magento give me test instance

AndresInSpace avatar Aug 28 '24 18:08 AndresInSpace

Hi @AndresInSpace. Thank you for your request. I'm working on Magento instance for you.

Hi @AndresInSpace, here is your Magento Instance: https://45243a10eb203fde7e8eedf63293cfe1.instances-prod.magento-community.engineering Admin access: https://45243a10eb203fde7e8eedf63293cfe1.instances-prod.magento-community.engineering/admin_5628 Login: b9724f7c Password: 835839f0d1f0

Hi @AndresInSpace, here is your Magento Instance: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering Admin access: https://0a05fa17a7983d4b098002b067de8a5d.instances-prod.magento-community.engineering/admin_d3bb Login: 40e0096b Password: 7f4a06ea70f6

@magento give me 2.4.7 instance with extensions adyen/module-payment:9.5.3

fabiopelloso avatar Sep 25 '24 11:09 fabiopelloso

Hi everyone, any ideas of when this will be merged? We've been suffering lately due to this with the page builder module and generally mixins not always loading preventing saving cms blocks and pages.

Applying this patch temporarily seems to solve the issues.

ioweb-gr avatar Nov 15 '24 10:11 ioweb-gr

@Den4ik you self-requested a review 2 months ago -- is there anything left to be reviewed to push this fix forward? Thank you.

medigeek avatar Nov 29 '24 19:11 medigeek

Hi @AndresInSpace,

Could you please recheck this PR and resolve the merge conflicts when you have a moment? It's highly anticipated and would be a valuable addition to the community!

Thank you for your efforts :)

ioweb-gr avatar Jan 18 '25 08:01 ioweb-gr

@Den4ik Hi, I updated instead of @AndresInSpace and fixed the conflicts in a fork: https://github.com/magento/magento2/compare/2.4-develop...medigeek:magento2:pr-39097-rebased-fixed-conflicts-jan-2025 https://github.com/medigeek/magento2/tree/pr-39097-rebased-fixed-conflicts-jan-2025

most stuff in require.js have already been fixed previously, but the other files weren't. hope it helps.

medigeek avatar Jan 19 '25 09:01 medigeek

Hi @medigeek Thanks for update. Could you please merge your branch into AndresInSpace:requirejsfix to proceed with current PR.

Den4ik avatar Jan 20 '25 09:01 Den4ik