-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Chrome: Load more than 10 tags/categories #1490
Conversation
6301959
to
9424e08
Compare
@jasmussen weird, I can't reproduce these errors. Any particular step to follow? |
I suspect this may be one of them docker/nginx rest api issues like I had with publishing back in the day. Recorded this video: |
@jasmussen mmm right, did you try to permalink trick? |
@jasmussen can I ask you to expand these failing network responses to see the full response? |
Here's a video showing me looking at some network stuff: https://cloudup.com/cSAEwSl9XI5 Looks like two request types were failing.
Request headers:
Response headers:
Not sure how helpful that is, but hope it helps a bit. Let me know if I can help in any other way. |
I see some cancelled requests on the recording. These are not bugs, I explicitly cancel "search" requests if the user types something else. But the 500 errors while creating tags, that's an issue, could you hit the |
Checked. Here's the response:
Video: https://cloudup.com/ccYPcpcmkva So yeah, looks like my monkey fingers tapping in the same tags over and over again is causing headaches. But that shouldn't cause 500s and js errors should it? |
@jasmussen ok Got it now, Thanks for the details. I'll try a fix :) |
So the issue is hard to reproduce, you need to try to add an existing tag than hasn't been fetched yet. I pushed a fix, you may still see failing requests but they are correctly handled now, if the term already exists we fetch it instead of creating it. |
@jasmussen Yes this is normal. If you add tags manually we trigger a request to create these tags, if they already exist the network request will fail, but it's caught properly and we trigger a "fetch tag" request to retrieve the existing corresponding tag. We can't do the alternative (fetching by name and creating if it doesn't exist because the API is not suited for it). |
If the js errors don't break anything and we're okay with them, then I'm okay with them too 😉 |
Is there no way we can not let it error? |
@iseulde I'm catching the failed promise, I guess browsers display all failing network calls. Open to suggestions |
Anyway, I think we should merge this since it resolves several problems. |
A couple of things here:
|
|
OK, sounds like it's not a big deal either way. We can get the HTTP 500 fixed though - please let me know if you see any more of those. More generally, there is a lot of AJAX logic in these components already, and the logic here isn't very ____able. It looks like we're ready to implement a more generic data layer/strategy and refactor this. |
I totally agree. we'd need to settle on an approach for the Data Layer, Maybe QueryComponents and effects is the most logical approach even if I'd love GraphQL (not battle tested yet). Let's make a decision in #902 |
I reported the HTTP 500 error here at https://core.trac.wordpress.org/ticket/41370 |
closes #1338 #1431
The REST API can't return all the tags/categories in one request, this makes writing selectors very hard. So here are the fixed proposed in this branch:
For the categories selector, we'd have to rewrite completely (maybe with virtual scroll, something like Calypso) to load paginated categories but I think this is sufficient for now.