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

fixes requireJS mixins racecondition with require calls in dom #39097

Open
wants to merge 5 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

AndresInSpace
Copy link

@AndresInSpace AndresInSpace commented Aug 23, 2024

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

checkLoadedTimeoutId = setTimeout(function () {

See more information in my comment on the issue this PR should fix below.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes jQuery Widget or UiComponent mixin don't loading or loading randomized #33593
  2. Properly Fixes JS error in the customer data #31920
  3. Addresses [ERROR] Failed to load the "Magento_Checkout/js/view/form/element/email" component #38274 (config output may need hoisting see my comment in ticket)
  4. Should also fix Mixins are not applied for bundled modules 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 (*)

  • Pull request has a meaningful description of its purpose
  • 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)

Copy link

m2-assistant bot commented Aug 23, 2024

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

❗ 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
ℹ️ 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-community-project m2-community-project bot added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S1 Affects critical data or functionality and forces users to employ a workaround. labels Aug 23, 2024
@AndresInSpace AndresInSpace changed the title fixes requireJS mixins racecondition with require calls in dom, ensures fixes requireJS mixins racecondition with require calls in dom Aug 23, 2024
@AndresInSpace
Copy link
Author

AndresInSpace commented Aug 23, 2024 via email

Copy link

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

Copy link

@AndresInSpace
Copy link
Author

@magento give me 2.4-develop instance

Copy link

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

Copy link

@AndresInSpace
Copy link
Author

AndresInSpace commented Aug 23, 2024

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
Copy link
Author

@magento run all tests

@mrtuvn mrtuvn added Area: Frontend Component: RequireJs Area: Perf/Frontend All tickets related with improving frontend performance. labels Aug 24, 2024
@AndresInSpace
Copy link
Author

@magento give me test instance

Copy link

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

Copy link

@AndresInSpace
Copy link
Author

@magento give me test instance

Copy link

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

Copy link

@AndresInSpace
Copy link
Author

@magento give me 2.4-develop instance

Copy link

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

Copy link

@AndresInSpace
Copy link
Author

@magento give me 2.4-develop instance

Copy link

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

@AndresInSpace
Copy link
Author

@magento give me test instance

Copy link

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

Copy link

Copy link

based on acceptance of requirejs/requirejs/pull/1859 removes nextTick cause of race condition
@Den4ik Den4ik self-requested a review September 18, 2024 07:05
@fabiopelloso
Copy link

fabiopelloso commented Sep 25, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Area: Perf/Frontend All tickets related with improving frontend performance. Component: RequireJs Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: review Severity: S1 Affects critical data or functionality and forces users to employ a workaround.
Projects
None yet
4 participants