Skip to content

RequireJS resolver sometimes fails to detect blocked resources #28117

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

Merged
merged 5 commits into from
Sep 21, 2020
Merged

RequireJS resolver sometimes fails to detect blocked resources #28117

merged 5 commits into from
Sep 21, 2020

Conversation

ishakhsuvarov
Copy link
Contributor

@ishakhsuvarov ishakhsuvarov commented May 5, 2020

Description (*)

Under certain circumstances Magento's RequireJS resolver would not correctly detect that all resources on the page were loaded or handled appropriately and cause "infinite loader" conditions. This can possibly be reproduced on Checkout

Also, it's sometimes possible to reproduce this condition by having magento/module-data-services module installed and using uBlock origin adblocker.

Related Pull Requests

N/A

Fixed Issues (if relevant)

Fixes #28116

Manual testing scenarios (*)

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)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented May 5, 2020

Hi @ishakhsuvarov. Thank you for your contribution
Here is some useful tips 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

For more details, please, review the Magento Contributor Guide documentation.

@ghost ghost added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels May 6, 2020
@VladimirZaets VladimirZaets added this to the 2.4.1 milestone May 6, 2020
@VladimirZaets VladimirZaets linked an issue May 6, 2020 that may be closed by this pull request
@sdzhepa sdzhepa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label May 18, 2020
@slavvka
Copy link
Member

slavvka commented May 19, 2020

@magento run all tests

@ihor-sviziev ihor-sviziev self-assigned this May 26, 2020
@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ishakhsuvarov,
Your changes looks good to me. Could you fix static tests and cover your change with js unit test?

@VladimirZaets
Copy link
Contributor

@magento run all tests

@ihor-sviziev
Copy link
Contributor

@ishakhsuvarov will you be able to update your PR?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jun 15, 2020

@engcom-Charlie @engcom-Kilo @engcom-Golf could you help with test coverage? it seems to me as really important fix

Thank you!

@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Jun 15, 2020
@engcom-Kilo engcom-Kilo self-assigned this Jun 15, 2020
@ihor-sviziev
Copy link
Contributor

@engcom-Kilo any updates on it?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jul 29, 2020

@magento run all tests.

I believe with JS unit test it will be not really easy to cover such case, so maybe it's better to keep it not covered, but have the fix, than not having such important fix at all.

@ihor-sviziev ihor-sviziev removed the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Jul 29, 2020
@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Jul 29, 2020
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7914 has been created to process this Pull Request

@engcom-Kilo engcom-Kilo removed their assignment Aug 17, 2020
@sidolov sidolov removed this from the 2.4.1 milestone Aug 20, 2020
@engcom-Alfa engcom-Alfa added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Aug 31, 2020
@engcom-Hotel engcom-Hotel self-assigned this Sep 18, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

A manual scenario was used to test the issue #28116

Before:

✔️ Ad Blocker Extension blocks script download from the original url

✖️ Checkout page is displaying 2 spinning loaders and a fully rendered page under it as resolver does not track components which were loaded via fallbacks.

Peek 2020-09-21 10-19

After:

✔️ Ad Blocker extension blocks the script download from the url stated in RequireJS config (https://example.com/advertisement.js)

✔️ Checkout page loads and renders just fine

Peek 2020-09-21 10-27

@m2-assistant
Copy link

m2-assistant bot commented Sep 21, 2020

Hi @ishakhsuvarov, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Lib/Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests help wanted Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

RequireJS resolver sometimes fails to detect blocked resources
10 participants