Frontend: GitHub API requests may be invoked twice on package details page

[jayaddison]

Describe the bug
On the package details page (for example, https://test.pypi.org/project/example/ ), the frontend JS application may make multiple requests to the GitHub API if the package contains URLs that look like links to GitHub repositories.

Expected behavior
A single network call to the GitHub API should occur for statistics retrieval.

To Reproduce

• Monitor outbound HTTP requests (for example, by enabling built-in web browser developer tools)
• Visit https://test.pypi.org/project/example/ (a package that contains a GitHub URL reference)
• Observe that multiple requests to the host api.github.com occur

My Platform
Firefox 102, Linux

Additional context
The controller code that manages this appears to load twice - maybe that could be the cause?

Also: multiple-connect-calls appears to be an expected behaviour of the stimulus library that the controller uses: hotwired/stimulus#215 (comment) - perhaps migrating to the initialize function could help?

[di]

Thanks for this issue, confirmed that this indeed happens.

[jayaddison]

The cause for this seems to be that the project-data.html template is included twice on the package detail page:

• Once in the sidebar
• Once in the vertically-arranged tabs

That means that two controllers exist on the page. Each one is called once when the application's controllers are loaded.

I don't yet have any recommendations for how to resolve that. Naively it seems like we should make one HTTP request before initializing the controllers -- or somehow add synchronization so that both controllers block on a shared, single HTTP request (and both utilize the same response data).

[jayaddison]

From what I understand about @hotwired/stimulus so far (not a lot, but learning), a way to achieve this could be to place a single "repo info retrieval" controller into the DOM that is a parent of the "repo info display" controllers.

For communication between controllers, DOM events are often recommended. So it would be possible for the (singleton) info-retrieval controller to perform the API request, store the relevant data in the DOM (for example, as data attributes), and then emit an event when that data is ready for consumption by the (multiple) info-display controllers.

It would be important to make sure that the info-display controllers are subscribed to the relevant DOM event before the API response is received, so perhaps the flow-of-control could be (pseudocode):

• info-retrieval.initialize: noop
• info-display.initialize: subscribe(event)
• info-retrieval.connect: fetch(data) ... emit(event)

Confirming the behaviour and timing of those calls would be important (subscribes must occur before emit, regardless of fetch latency).

[jayaddison]

@di would you hypothetically accept a pull request to resolve this that uses no-framework browser JavaScript?

(I'm wondering whether it'd be more straightforward to make a single HTTP fetch from the global context and then query and update relevant elements in the DOM when that returns)

[di]

I think a) this isn't a huge problem as it stands right now, and b) we're generally trying to move away from doing JS outside of our frontend framework.

So I wouldn't not merge it, but it's probably just not a great use of your time!

[miketheman]

Thanks @jayaddison for tracking that down - I noticed it and didn't understand where it came from.

I'm thinking the "actual problem" is that we include the HTML twice in the same page, and control which is visibile based on viewport. This bloats the HTML and triggers this behavior - since the controller is included twice, it's invoked twice.

A better solution would be to reevaluate the grid/layouts in use and figure out how to better express both the desired desktop and mobile layouts reusing the same HTML blocks, and thus only having a single controller.

[jayaddison]

Thanks @miketheman - agreed, consolidating the two navigation component lists into one would be a nice improvement here.

I'm also exploring a shorter-term fix with the existing debounce module: declaring a module-scoped debouncedFetch function that both controller instances call, meaning that in most cases only a single outbound fetch would occur.

[jayaddison]

I'm also exploring a shorter-term fix with the existing debounce module: declaring a module-scoped debouncedFetch function that both controller instances call, meaning that in most cases only a single outbound fetch would occur.

This approach didn't seem great to me after exploring it a little in #12773.

Based on a sense that consolidating the two navigation lists could be tricky (both implementation-wise and maintenance-wise), I think that the next alternative to explore could be:

• Upgrade to stimulus v3.2.0 or later to gain support for Outlets
• Use Jinja2's with statement (built-in since v2.9 -- we are currently on v3.1.2) to pass a unique id for each repo-info controller
• Create a repo-statistics-gather (or similar) controller
• Attach both repo-info controllers by-identifier as outlets of the repo-statistics-gather controller
• Move the API request to the statistics-gathering controller, and have it pass the relevant statistical fields to the repo-info controllers to display