Problem/Motivation

All Project Browser instances on a single page, should use the same query manager so that we can display the same project consistently across multiple PB instances.

Steps to reproduce

Proposed resolution

  • Handle all project data fetching by moving the load() function from ProjectBrowser.svelte to QueryManager.load().
  • Cache results in-memory
CommentFileSizeAuthor
#10 flickering.mp41.06 MBchrisfromredfin
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

narendrar created an issue. See original summary.

narendrar’s picture

Title: Move load() function of ProjectBrowser.svelte into a new window-scoped class » Refactor Project Data Fetching into a new Window-Scoped QueryManager
Issue summary: View changes
narendrar’s picture

Status: Active » Needs review
phenaproxima’s picture

That's pretty much exactly what I imagined! I have a couple of questions, and I think we need to be sure we're documenting this design properly, or it won't be clear to anyone why we have this window-scoped singleton.

narendrar’s picture

Thanks for the review, @phenaproxima! I’ve addressed the feedback and made some refinements.

phenaproxima’s picture

Title: Refactor Project Data Fetching into a new Window-Scoped QueryManager » Load project data from a new QueryManager class that implements a centralized, static cache
Issue summary: View changes
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Okay - tentatively RTBCing. I think this is the final step before we need to do the bigger front-end lift, which is...somehow make all project objects react automatically to changes in the centralized cache. That's going to take some thinking and probably a fair amount of refactoring.

But, once this lands, all the other pieces are in place.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

I analyzed this a bit more and decided to proceed with turning QueryManager into a store (i.e., it fulfills Svelte's "store contract" by implementing a subscribe() method in a particular way). The idea is that it should invoke callbacks whenever project data in the centralized cache has changed.

chrisfromredfin’s picture

Status: Needs review » Needs work
StatusFileSize
new1.06 MB

A few comments:

  1. This introduces quite a bit of flickering in the UI that I think is a UI regression/bug.
  2. When activating multiple projects in one fell swoop, it seems to only mark the first one as "Installed." However, the front-end seems to be reacting to the correct statuses from the backend, since the `/activate?_wrapper_format=drupal_ajax` being returned is showing "active" for the first result, but only "present" for the remainder in the list. Note that if you mix-and-match; that is, if you pick two that are already in the filesystem and two that need to be required in, then it activates the local ones first (in the same way, only first one is reflected correctly in the UI), then it activates the required ones (and again shows same behavior, only the first one of that "batch" is reflected correctly in the UI). And again, this is because of what's coming back from the AJAX system. I believe the UI is properly doing "what it's told," but it's being told wrong.
phenaproxima’s picture

Status: Needs work » Needs review

OK, I think this will improve things.

  • I can't quite reproduce the flickering, but I suspect that using Svelte's indexing to help reduce diffing will improve it. So I tried that.
  • The module activation bug is legitimate and was related to the fact that ModuleActivator is holding on to an outdated container. I fixed this by forcing it to pull the new module handler and module list from the global service wrapper. This is a known pitfall of the container being changeable during a single request (and indeed, if activating multiple projects at once, that is likely to happen). I confirmed in manual testing that it is fixed now, but added a low-level test for good measure.

tim.plunkett made their first commit to this issue’s fork.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Reverted my commit, will address in follow-up. This is good to go

tim.plunkett’s picture

Saving credit

tim.plunkett’s picture

Status: Reviewed & tested by the community » Fixed

Merged !747

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.