Fix theme serve hot reload when there are many tabs active

[karreiro]

WHY are these changes introduced?

When users have two tabs active and perform a change in some liquid file, both tabs get refreshed; however, only one gets the updated version:

That's happening because two requests reach ShopifyCLI::Theme::DevServer:: Proxy, but at this point, the first request goes in the first branch of this condition, and the second one goes to the second branch.

The replace_templates is different for each request because @syncer.pending_updates has an item for the first one, but it's empty for the second request.

WHAT is this pull request doing?

This draft PR handles this is issue by keeping params@templates_to_replace persistent. However, this solution is not great. @templates_to_replace may get too large, which implies too large requests in the SFR.

How to test your changes?

1. Initialize a new theme with shopify-dev theme init my_theme
2. Serve the new theme with shopify-dev theme serve
3. Open two Google Chrome tabs at http://127.0.0.1:9292
4. Perform some change in the layout/theme.liquid file

Post-release steps

None.

Update checklist

• I've added a CHANGELOG entry for this PR (if the change is public-facing)
• I've considered possible cross-platform impacts (Mac, Linux, Windows).
• I've left the version number as is (we'll handle incrementing this when releasing).
• I've included any post-release steps in the section above.

[karreiro]

Hey @macournoyer! :)

As the PR description mentions, this PR still needs some enhancements.

I believe we need to keep @templates_to_replace, so, for the final fix, we could adopt different alternatives for cleaning it. With that in mind, I would propose these two alternatives:

1. Clean based on page content matches

• Introduce a second unblocking/async request (this request would be similar to this one)
• Trigger this second unblocking/async request every time we pass modified templates to the SFR
• Finally, clean @templates_to_replace entries when the response body of these two requests match

2. Clean based on timestamps

We could clean @templates_to_replace entries older than 5 seconds (based on their timestamp).

What do you think about these approaches? (I personally prefer the second one 🧐).

[karreiro]

Thank you for the awesome clues, @macournoyer! 🚀

Thinking about the coupling between the Proxy with the Syncer, I've got an idea.

What if we pass the modified files (["layout/theme.liquid"]) as a parameter in the hot-reload.js (the event already contains that information):

For now, we could merge the files from the new parameter and @syncer.pending_updates, and we would bring params back as a local variable. Also, the Proxy would start getting more independent from the Syncer.

What do you think about this approach?

[karreiro]

@macournoyer, I've updated the PR with a draft of the idea above for clarifying it.

My first implementation relied on a query parameter, but I've changed it to a cookie to keep it more transparent for users.

[macournoyer]

I think my brain is fried... I understand why it works, it kind of mimics a delay, right?

I'm not sure if this is a hack, or if a real delay should be used.

Code looks good, so I'm trusting your on reasoning behind this! 🎄 🍸

[karreiro]

Thanks for the review, @macournoyer! 🚀

The delay is not so significant, as we redefine the hot_reload_sections when necessary (in the hot-reload.js#refreshPage function). It's intended to be used when users manually refresh the page.

The nice thing is that now we've empowered the client to update the templates parameter (and we may drop the syncer dependency in the future).

Thanks again for the review! I've added the missing unit tests to merge it 😊