Fix hot-reload early trigger

[tommypepsi]

WHY are these changes introduced?

Fixes #1409

The hot-reload can be triggered too fast because it is triggered as soon as the file is modified instead of waiting for the file to finish uploading when necessary.

WHAT is this pull request doing?

It adds the syncer to the HotReload class and adds an observer to it. The syncer sends a message to the HotReload when the queue becomes empty after performing a task notifying that the upload is finished.

The hot-reload.js file stops making the assumption that only one file is modified at a time (multiple files can be modified at a time with build tools). If a file cannot be dynamically hot reloaded it will start waiting for the file uploadComplete message from the event source before reloading the page.

[pepicrft]

Thanks a lot for improving this @tommypepsi. I left some comments that it'd be great to get addressed before merging the PR.

[pepicrft]

Thanks a lot @tommypepsi for tackling this :)

[tommypepsi]

Thanks a lot @tommypepsi for tackling this :)

And thanks for taking a look @pepibumur! Seems there's another step since it's my first contribution

[tommypepsi]

@pepibumur Any possibility of moving this forward now that it's approved? It is definitely a big pain point developing themes without the fix and can't wait to finally have that part of the official shopify CLI 😅

[tommypepsi]

@pepibumur it seems we need a maintainer to move this PR forward

[macournoyer]

Absolutely fantastic @tommypepsi 👏 Very simple solution!

I'm marking as "Request changes" because of the regression for .js files. They are also served locally, so no need to wait for upload.

Thanks so much for sending this ❤️

[macournoyer]

I think this should be inside the if? Or else it will never be reseted to false.

EDIT: However, I think because the page is reloaded it might not be a problem. But still I would move inside the if to make this clearer.

[tommypepsi]

The thing is once it switches to true it needs to stay true until we receive the "uploadComplete" message.

While we are waiting for files upload other files can still get modified so we don't want it to be set to false because a file that needs a full refresh has changed before so we still need to wait for the upload to complete.

This is why I have set it outside the function. I've also put it outside of the function because the code to reload a section seems to assume that some stores might not support the section rendering api so we need to set the nonDynamicFileChanged to true from outside the handleUpdate function (But I feel like this is not relevant anymore, I think all store have access to it now).

[macournoyer]

It should also include .js files, as those are also served locally.

[tommypepsi]

Now that you mention that it makes me realize that sections are probably not served locally since shopify needs to parse the liquid. Considering that I think we should wait for the uploadComplete message for sections also. Otherwise it will call the section rendering api and won't get the changes if the file hasn't finished uploading.

[tommypepsi]

I did more tests with the sections and indeed the sections are hot reloading too fast for the file upload.

I noticed two things with the hot-reload of the sections:

• The hot reload for sections currently only works for hardcoded sections because it's targeting the sections like that:
  this.element = document.querySelector(`[id^='shopify-section'][id$='${this.name}']`);
  this.name is only present in the id when the section is hardcoded. So it doesn't work with json templates because their ID looks like that: shopify-section-template--15051264917712__1628614367b966b55d

• When fetching the section through the section rendering API it checks if the header x-templates-from-params is present to see if the store supports the API but from my tests this doesn't seem to be present even if the store supports the section rendering api. When this header is not present it falls back to reloading the page.

Locally I managed to fix and make it work with the json templates! But somehow I'm having a new problem. The first time you make a change after loading the page the hot reload for sections seems to be happening too fast even if you wait for the "uploadComplete" message. But subsequent change seems to be ok. My guess is that even if you get the "uploadComplete" message it doesn't mean that shopify has finished parsing the file or maybe it's a cache thing. (Though I'm really not sure why the problem would only happen on the first change after the page load)

For now I've added only the change for the .js files. Not sure if I should add my changes for the sections, it's mostly working except for this weird bug. I'll wait for your opinion on that.

[macournoyer]

We have a special Proxy that does a trick to make sure Liquid files are updated faster in the backend.

It passes the body of the templates with the request, so the backend doesn't have to wait for the file to be updated: https://github.com/Shopify/shopify-cli/blob/main/lib/shopify_cli/theme/dev_server/proxy.rb#L126.

This should take care of the section templates, and we shouldn't have to wait for them to be updated.

You are correct that uploadComplete doesn't mean the backend has finished processing the file. I don't have a good solution for that.

[tommypepsi]

If everything makes sense for you I think this PR could move forward like this and the sections hot-reload part could be handled in a separate PR.

Currently the sections triggers most of the time a full refresh and the full refresh seems slow enough that we don't see the problem I was talking about.

[tommypepsi]

I did a couple of tests with the proxy and the "replace_templates" form param that is passed to the proxy doesn't seem to help. The request always returns the section with the previous changes so it's always a change behind. (Is it possible that this has been removed from the backend since it is not documented and that we are seing a lot of changes lately with OS 2.0?)

If I wait for the uploadComplete message it seems to work most of the time though. The only thing is that it doesn't work the first time after a manual refresh the page, changes after that work as intended. For some reason this only happens when you manually refresh, when the hot reload does the refresh it works as intended somehow.

Edit: now that I think about it the fact that the proxy trick doesn't work anymore might be the main reason why my changes are needed. My changes would still fix some small cases but the biggest pain point comes from liquid files not updating fast enough which is not supposed to be the case with the proxy trick.

[macournoyer]

My changes would still fix some small cases but the biggest pain point comes from liquid files not updating fast enough which is not supposed to be the case with the proxy trick.

Aaaah! Indeed it shouldn't be the case. The Proxy should handle all that and make the changes instant. Sorry for not catching that earlier.

Is this issue only affecting a particular set of Liquid files, or every change you make to any Liquid files has this delay in being updated?

I just tested, and it works for me, so the issue is likely be somewhere else. Can you reproduce with the environment variable DEBUG=1 set, and paste the logs. It should show if the file is being updated properly.