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

QueryDispatcher/Request.js refactor #5028

Merged
merged 9 commits into from Aug 24, 2022
Merged

QueryDispatcher/Request.js refactor #5028

merged 9 commits into from Aug 24, 2022

Conversation

boltzmannsBane
Copy link
Contributor

@boltzmannsBane boltzmannsBane commented Jul 13, 2022

In this PR:

  • Refactored QueryDispatcher/Request.js using async/await syntax
  • Fixed error handling & formatting
  • Made methods more semantically precise - methods only do what their method name suggests they are

@aws-amplify-eu-west-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-5028.d25kd40decjpue.amplifyapp.com

@aleksandrakorolova
Copy link

  1. Can reproduce issue from the task:
  • URL for Men category
  • Category Description for Tech category
  • Products from Women category
    Screenshot 2022-07-13 at 15 21 28

2.Error fetching messages don't appear BUT Error message 'Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'message')' appears in the console

@aleksandrakorolova
Copy link

  1. Still can reproduce different cases:
  • URL and category description are displayed for category X but products are displayed for category Y
    Screenshot 2022-07-18 at 15 55 55
  • URL and products are displayed for category X but category description for category Y
    Screenshot 2022-07-18 at 15 59 21
  • URL is displayed for category X but category description and products for category Y
    Screenshot 2022-07-18 at 16 08 52
  1. Fixed

@boltzmannsBane boltzmannsBane changed the title 3731 - wrong content on page switching w/ slow internet - fixed WIP [3731] - wrong content on page switching w/ slow internet - fixed Jul 19, 2022
@boltzmannsBane boltzmannsBane changed the title WIP [3731] - wrong content on page switching w/ slow internet - fixed QueryDispatcher/Request.js refactor Jul 20, 2022
@boltzmannsBane
Copy link
Contributor Author

boltzmannsBane commented Jul 20, 2022

Relevant for #3731 and #3088:
Those issues are a byproduct of our caching strategy.
I initially planned to cancel a request if a newer request was dispatched before the previous one completes. The problem is that the cache is... to fast. Getting a response from the cache only takes 2-3ms, which is way faster than a human is physically able to click on a new link to dispatch a new request and cancel an older one.
This becomes problematic when the service worker, which sends requests to an actual server, on slow 3g takes anywhere from 200ms-6s to complete a request. A user can queue multiple uncancellable requests like that, and they all will resolve and attempt to update the app state, which leads to the behavior described in the aforementioned issues.

This PR won't solve the issues but it still has a lot of good code, reactors, adjustments, etc, so I believe we could still try to merge it.
How to test: this PR doesn't add anything new behavior-wise. As long as homepage/pdp/plp/cart/checkout work as intended, it's safe to merge.

@Ylmzef
Copy link

Ylmzef commented Aug 23, 2022

I'm still facing with this issue in 5.3.3

@zans-laksa zans-laksa merged commit 2d8c751 into scandipwa:master Aug 24, 2022
@sliceem88
Copy link

Getting error on search with this fix:
code: "410" error: true message: "Query hash is unknown"
image

Rolled back to 5.3.3

@boltzmannsBane
Copy link
Contributor Author

@Ylmzef this PR won't fix the issues, refer to the earlier comment

@sliceem88 Is the search result broken as well? If it's just the log, you're most probably seeing it because of better error handling and the cause of the error exists in the older versions too. There are also a few edge cases that cause the search to fail that are unrelated to this PR.

@Ylmzef
Copy link

Ylmzef commented Aug 30, 2022

Getting error on search with this fix: code: "410" error: true message: "Query hash is unknown" image

Rolled back to 5.3.3

Same here

@sliceem88
Copy link

sliceem88 commented Aug 31, 2022

@Ylmzef this PR won't fix the issues, refer to the earlier comment

@sliceem88 Is the search result broken as well? If it's just the log, you're most probably seeing it because of better error handling and the cause of the error exists in the older versions too. There are also a few edge cases that cause the search to fail that are unrelated to this PR.

Yes, search doesn't work. Also some random PDP.

Any reason why rolled back worked ?

@Ylmzef
Copy link

Ylmzef commented Aug 31, 2022

Yes, I rolled back to 5.3.3 and it's working fine again.

@Ylmzef
Copy link

Ylmzef commented Sep 8, 2022

is there any progress yet? Our website not available to customers for a long time due this issue.
Maybe can you release a patch for this issue? It’s not a simple problem for live shops.

@boltzmannsBane
Copy link
Contributor Author

@Ylmzef the fix can be found here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants