Opened 9 years ago
Closed 8 years ago
#36697 closed defect (bug) (fixed)
Combining the HTTP requests for obtaining the available nav menu items into a single request
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 4.3 |
Component: | Customize | Keywords: | needs-testing has-patch has-unit-tests |
Focuses: | Cc: |
Description
Have many admin-ajax.php are called ( total depend on post types and taxonomy types are defined ) at same time when click on Menu from Customizer or Shift + click on menu from Preview Window that cause take many resource from server
Attachments (6)
Change History (36)
This ticket was mentioned in Slack in #core-customize by nguyencongtuan. View the logs.
9 years ago
#2
@
9 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#5
@
8 years ago
- Keywords has-patch needs-testing dev-feedback added; needs-patch removed
Patch above uses most of the original code and simply puts iterators in where needed. Keen for someone to review and help me align with WordPress preferred syntax.
Also, open to suggestions for a cleaner way to manage the loading state of availableMenuItemContainer.
#6
@
8 years ago
- Keywords needs-refresh added
@curdin thanks for the patch! Would you mind refreshing it? It's not applying anymore, probably due to [38436].
#7
@
8 years ago
- Keywords needs-refresh removed
@westonruter See above for the refreshed patch. I just got a whole lot better at fixing git merge conflicts :)
#8
@
8 years ago
I was just thinking about this some more today and I realized that I don't really understand why all of the available items need to be loaded up-front to being with. For one thing, the available items requests are being made when the menus panel is opened, but these could be wasted requests because they'll never be displayed if the user never opens the panel to chose from available nav menu items.
Secondly, then the user goes to “Add Items” the panel that opens shows all of the available item types in _collapsed_ accordions. None of the items are displayed there either. Would it not be much more efficient to just make the Ajax request to fetch the items of a given type when the accordion section is expanded? The spinner can be shown there initially to show that work is being done.
@celloexpressions thoughts?
(Another thought: If we do go ahead and combine all available nav menu items into a single request, there is a slight chance that there could be a server timeout if there are many custom post types and taxonomies. That being said, it's less likely to be problematic compared to hitting the server with X as many HTTP requests.)
#9
@
8 years ago
36697.individually-deferred.diff is an initial patch that loads the items for a given type the first time that the accordion section is opened. Seems like some refactoring should be done to move the data
attributes to the top li
so that a secondary query for .available-menu-items-list
could be avoided.
#10
@
8 years ago
- Version changed from 4.5 to 4.3
From the end user perspecitve, it's important that it seems like the content is already there when they open the panel. There can't/shouldn't be a delay between opening a section of available menu items and seeing at lest the initial items there to select from.
It might partially be because we fire so many requests at once, but even with the current setup I often have to wait for sections to finish loading by the time I get to opening them. Merging these into one request is probably the best approach to improving the user experience here. The vast majority of users won't care about content loading that they might not need/see; a corollary is the fact that most customizer objects are loaded (at least their data) when the customizer is opened even if they won't be used. They'll care if they have to wait for something to load, and anything that has to do an Ajax call has the potential to be quite slow and even to fail, even in my everyday experience with sub-par (for the US) connections.
In most cases the user will be opening the add-items panel when they go into menus, with the exceptions being changing locations, reordering, or deleting only, although those actions often also coincide with adding items. We're not talking about a lot of data here, so if we can start loading it in anticipation of the user needing it, the user is less likely to need to wait. The approach in 36697.2.diff seems like the best improvement we can make here.
#11
@
8 years ago
- Keywords dev-feedback removed
- Owner set to westonruter
- Status changed from new to accepted
Fair enough, although the reason for why the customizer overall can be very slow to load is that too much is loaded up-front and not enough is lazy-loaded as needed (see #28580). In the case of nav menu items, loading all in one request should suffice as long as it doesn't block the initial page load, which it wouldn't here.
#12
@
8 years ago
@curdin I'm having trouble applying your patch. It seems the diff file is malformed:
patching file src/wp-admin/js/customize-nav-menus.js patch: **** malformed patch at line 6:
Could you point me to the Git repo you are using or re-attach a patch that applies cleanly?
This ticket was mentioned in Slack in #core by stevenkword. View the logs.
8 years ago
#15
@
8 years ago
@westonruter believe this can be done in Beta for 4.7, but it looks like we are still waiting on a patch refresh.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#19
@
8 years ago
- Keywords needs-refresh added
Looks like we're still pending an updated patch from @curdin here.
#20
@
8 years ago
Hello folks, apologies I'm just back from 6 weeks of holidays - will refresh shortly.
#22
@
8 years ago
- Keywords needs-refresh removed
@westonruter refreshed patch added. Apologies for the delay but I had an excellent holiday :)
#23
@
8 years ago
- Keywords needs-patch needs-unit-tests added; has-patch removed
@curdin thanks for that.
I've opened a pull request for review and additional revisions. Here are the first few changes I've made: https://github.com/xwp/wordpress-develop/pull/189/files/b0c89dc..30f3739
A couple other outstanding issues that need to be fixed:
- Pagination is broken when infinite scrolling through available nav menus of a given type. Fetching of new items for a given type needs to be updated to use the new request signature. So
loadItems
needs to work both with single type and multiple types. - PHP unit tests are broken.
Instead of modifying the existing Ajax request to require multiple item types, I think that perhaps loadItems
should be smarter about whether its first argument is an array of itemType
objects or a single itemType
object. In the latter case, it can make the request with the object
and type
params as previously, and this would ensure backwards compatibility and should prevent the tests from breaking. But an array of itemType
objects are passed, then this is when the Ajax request can be made with the an item_types
array instead, and this is what would key in the new functionality to do the bulk request.
Is this something you can pick up? I'll attach my latest patch as well in case you don't want to collaborate on GitHub instead.
#25
@
8 years ago
@westonruter Can I have your opinion please? I've been working on this for a bit today and wanted to check if backwards compatibility in this case is strictly necessary. The reason is that the code could be simplified if we dropped compatibility. Eg see patch with backwards compatibility https://github.com/curdin/wordpress-develop/pull/1 vs patch without, where the ajax handler always expects an array of itemTypes https://github.com/curdin/wordpress-develop/pull/2
I'm not clear how widespread use for this function is outside core so looking for your expert opinion. I'll need to do some more work on whatever the preferred approach is.
#26
@
8 years ago
@curdin backwards compatibility is not a big concern for this Ajax handler. But I don't think we have to break backwards compatibility do we? I mean, the existing ajax handler is taking a type
and object
params. Could not the request to fetch multiple items just omit these and pass an array of item types instead?
#27
@
8 years ago
@westonruter sorry, I'm not sure I quite follow. I can certainly amend the loadItems
to take either type
and object
, or an array passed into type
. However the tests fail not because loadItems
has been amended, but because in my initial patch the ajax handler was changed to receive an array.
My preferred approach would be the following:
- 'loadItems' takes an array of itemType(s), if you prefer I can also leave the
type
andobject
params available - loadItems makes request to ajax handler using an array of itemTypes.
- Ajax handler returns an array of itemTypes and their items. This breaks the current tests as they expect a set of items for the requested itemType only.
loadItems
processes this array and injects data into DOM as required
Does this make sense? Give me a shout on Slack if not.
#28
@
8 years ago
@curdin I meant the PHP side of things. For the JS side of things we can maintain backwards compatibility by looking at the arguments
passed to loadItems
. If a second argument is passed, then we know that it's using the old signature of loadItems( type, object )
. If only one argument is present, then we'd know that it's using the new signature.
Ajax handler returns an array of itemTypes and their items. This breaks the current tests as they expect a set of items for the requested itemType only.
Unit test could be amended to only pass in the specific expected item type, right?
In any case, let me know which PR has your preferred approach and once you're ready I'll do a review and commit. It's probably too late for beta2 but it could be part of beta3 next week, unless you can hand it over to me in the next couple hours.
Agreed. The HTTP requests should be combined into a single request.
If anyone wants to own this we can milestone it for 4.6.