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

M2P-161 Stop pushing model/quote JS script #822

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

vitaliyreznikov
Copy link
Collaborator

@vitaliyreznikov vitaliyreznikov commented Jul 25, 2020

We introduced an issue in #796. Actually it was known magento issue that was fixed in new Magento versions, see magento/magento2#23099 , magento/magento2#14412, magento/magento2#21432

Under some conditions, Magento JS script produced an error because they initialated wrong.

We had a fix #808 but now I see it doesn't affect the root of the issue.

In PR 796 we added require(['Magento_Checkout/js/model/quote'] ...) thing. This requireJS API means we push model/quote script to load if it wasn't loaded before. Under some very rare conditions, it triggers incorrect initializing.

So solution the same as in magento/magento2#21432 : don't load module/quote script before DOM.loaded. We know that when DOM.loaded we have all parameters to initialize this script.

The issue about a race condition, so it's quite hard to reproduce it. Looks like it happens very rarely.
We need some (or all) of the following conditions:

  • site works slowly
  • browser work slowly (IE11 for example)
  • the first page load, so we don't have scripts in browser cache.
  • site works in developer mode

Fixes: [M2P-161]

#changelog M2P-161 Stop pushing model/quote JS script

Type of change

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please validate that you have tested your change in at least one of the following areas:

  • Successfully tested locally (or docker image)
  • Successfully tested on a staging or sandbox server
  • Successfully tested on a merchant's staging server

For PR Reviewer

  • Reviewed unit tests to make sure we are using real components rather than mocks as much as possible?
  • For any major change (observer, new Bolt feature, core Magento interaction) we must add a feature switch, did you verify this?

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • New and existing unit tests pass locally with my changes.
  • I have created or modified unit tests to sufficiently cover my changes.
  • I have added my Jira ticket link and provided a changelog message.

@vitaliyreznikov vitaliyreznikov changed the title Stop pushing model/quote JS script M2P-161 Stop pushing model/quote JS script Jul 25, 2020
@codecov
Copy link

codecov bot commented Jul 25, 2020

Codecov Report

Merging #822 into master will increase coverage by 0.17%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #822      +/-   ##
============================================
+ Coverage     92.86%   93.03%   +0.17%     
- Complexity     1378     1428      +50     
============================================
  Files           104      104              
  Lines          4668     4811     +143     
============================================
+ Hits           4335     4476     +141     
- Misses          333      335       +2     

Copy link
Contributor

@matt-thomason matt-thomason left a comment

Choose a reason for hiding this comment

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

What about the other two instances in this replacejs.phtml file where we rely on domReady!?

view/frontend/templates/js/replacejs.phtml Outdated Show resolved Hide resolved
view/frontend/templates/js/replacejs.phtml Outdated Show resolved Hide resolved
@vitaliyreznikov
Copy link
Collaborator Author

What about the other two instances in this replacejs.phtml file where we rely on domReady!?

Good point, but other things are work :) and we haven't changed them for months. Looks like only 'Magento_Checkout/js/model/quote' is brittle enough and we use it only in place I fix in this PR.

Maybe it makes sense to run all code after domReady, but I think it's too big change for hotfix PR. Let's discuss this during other JS refactoring tasks..

@matt-thomason
Copy link
Contributor

That is reasonable, thanks for talking it out.

@vitaliyreznikov vitaliyreznikov merged commit d02e104 into master Jul 27, 2020
@vitaliyreznikov vitaliyreznikov deleted the vitaliy/quote-listener-issue branch July 27, 2020 19:35
danac-gs pushed a commit that referenced this pull request Jan 26, 2021
* Stop pushing model/quote JS script

* Changes after review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants