Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Fix hot-reload early trigger #1458

Closed
wants to merge 12 commits into from
Closed

Fix hot-reload early trigger #1458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
50944d3
Add for upload complete to trigger page reload
tommypepsi Aug 18, 2021
5ccc3c2
Add variable to make notify_observers clearer
tommypepsi Aug 18, 2021
4d31fa2
Move hotreload log to not be logged multiple times
tommypepsi Aug 18, 2021
77d1d8c
Adds the wait for unsupported section reload
tommypepsi Aug 18, 2021
9d00034
Merge branch 'main' into fix/hot-reload-early-trigger
tommypepsi Oct 2, 2021
637350c
Add a test for the upload complete broadcast event
tommypepsi Oct 6, 2021
581113d
Change variable name to make it clearer
tommypepsi Oct 8, 2021
a4ff5c3
Remove unnecessary variable assignment
tommypepsi Oct 25, 2021
d3f197e
Merge branch 'main' into fix/hot-reload-early-trigger
tommypepsi Oct 26, 2021
8af899c
Add reload for js files
tommypepsi Oct 26, 2021
d5a6217
Change test to assert includes now that "uploadComplete" can be present
tommypepsi Oct 26, 2021
f2d34e1
remove rubocop trailing space
tommypepsi Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/shopify_cli/theme/dev_server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def start(ctx, root, bind: "127.0.0.1", port: 9292, poll: false)
# Setup the middleware stack. Mimics Rack::Builder / config.ru, but in reverse order
@app = Proxy.new(ctx, theme: theme, syncer: @syncer)
@app = LocalAssets.new(ctx, @app, theme: theme)
@app = HotReload.new(ctx, @app, theme: theme, watcher: watcher, ignore_filter: ignore_filter)
@app = HotReload.new(ctx, @app, theme: theme, watcher: watcher, syncer: @syncer, ignore_filter: ignore_filter)
stopped = false

theme.ensure_exists!
Expand Down
40 changes: 31 additions & 9 deletions lib/shopify_cli/theme/dev_server/hot-reload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,44 @@

connect();

let nonDynamicFileChanged = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

function handleUpdate(message) {
var data = JSON.parse(message.data);

// Assume only one file is modified at a time
var modified = data.modified[0];
if(data.modified) {
let containsJSFiles = false;

if (isCssFile(modified)) {
reloadCssFile(modified)
} else if (isSectionFile(modified)) {
reloadSection(modified);
} else {
console.log(`[HotReload] Refreshing entire page`);
data.modified.forEach(file => {
tommypepsi marked this conversation as resolved.
Show resolved Hide resolved
if (isCssFile(file)) {
reloadCssFile(file)
Comment on lines +28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@tommypepsi tommypepsi Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@tommypepsi tommypepsi Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else if (isSectionFile(file)) {
reloadSection(file);
} else if (isJSFile(file)) {
containsJSFiles = true;
} else {
nonDynamicFileChanged = true;
}
});

if(containsJSFiles && !nonDynamicFileChanged) {
console.log(`[HotReload] Refreshing entire page`);
return window.location.reload();
}
}

if(nonDynamicFileChanged) {
console.log(`[HotReload] Refreshing entire page, waiting for files to upload`);
}

if(nonDynamicFileChanged && data.uploadComplete) {
window.location.reload();
}
}

function isJSFile(filename) {
return filename.endsWith('.js');
}

function isCssFile(filename) {
return filename.endsWith('.css');
}
Expand Down Expand Up @@ -80,7 +102,7 @@

console.log(`[HotReload] Reloaded ${this.name} section`);
} else {
window.location.reload()
nonDynamicFileChanged = true;

console.log(`[HotReload] Hot-reloading not supported, fully reloading ${this.name} section`);
}
Expand Down
12 changes: 11 additions & 1 deletion lib/shopify_cli/theme/dev_server/hot_reload.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ module ShopifyCLI
module Theme
module DevServer
class HotReload
def initialize(ctx, app, theme:, watcher:, ignore_filter: nil)
def initialize(ctx, app, theme:, watcher:, syncer:, ignore_filter: nil)
@ctx = ctx
@app = app
@theme = theme
@streams = SSE::Streams.new
@watcher = watcher
@watcher.add_observer(self, :notify_streams_of_file_change)
@syncer = syncer
@syncer.add_observer(self, :notify_streams_of_upload_complete)
@ignore_filter = ignore_filter
end

Expand Down Expand Up @@ -40,6 +42,14 @@ def notify_streams_of_file_change(modified, added, _removed)
end
end

def notify_streams_of_upload_complete(complete)
tommypepsi marked this conversation as resolved.
Show resolved Hide resolved
@streams.broadcast(JSON.generate(
uploadComplete: complete
))

@ctx.debug("[HotReload] upload complete")
end

private

def request_is_html?(headers)
Expand Down
8 changes: 8 additions & 0 deletions lib/shopify_cli/theme/syncer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
require "thread"
require "json"
require "base64"
require "observer"

module ShopifyCLI
module Theme
class Syncer
include Observable

class Operation < Struct.new(:method, :file)
def to_s
"#{method} #{file&.relative_path}"
Expand Down Expand Up @@ -218,6 +221,11 @@ def perform(operation)
)
ensure
@pending.delete(operation)

if @pending.size == 0
tommypepsi marked this conversation as resolved.
Show resolved Hide resolved
changed
notify_observers(true)
end
end

def update(file)
Expand Down
20 changes: 17 additions & 3 deletions test/shopify-cli/theme/dev_server/hot_reload_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ def setup
root = ShopifyCLI::ROOT + "/test/fixtures/theme"
@ctx = TestHelpers::FakeContext.new(root: root)
@theme = Theme.new(@ctx, root: root)
@syncer = stub("Syncer", enqueue_uploads: true)
@syncer = Syncer.new(@ctx, theme: @theme)
@syncer.start_threads
@watcher = Watcher.new(@ctx, theme: @theme, syncer: @syncer)
end

Expand Down Expand Up @@ -67,7 +68,7 @@ def test_broadcasts_watcher_events
.with(JSON.generate(modified: modified))

app = -> { [200, {}, []] }
HotReload.new(@ctx, app, theme: @theme, watcher: @watcher)
HotReload.new(@ctx, app, theme: @theme, watcher: @watcher, syncer: @syncer)

@watcher.changed
@watcher.notify_observers(modified, [], [])
Expand All @@ -87,20 +88,33 @@ def test_doesnt_broadcast_watcher_events_when_the_list_is_empty
@ctx, app,
theme: @theme,
watcher: @watcher,
syncer: @syncer,
ignore_filter: ignore_filter
)

@watcher.changed
@watcher.notify_observers(modified, [], [])
end

def test_broadcast_upload_complete_event
SSE::Streams.any_instance
.expects(:broadcast)
.with(JSON.generate(uploadComplete: true))

app = -> { [200, {}, []] }
HotReload.new(@ctx, app, theme: @theme, watcher: @watcher, syncer: @syncer)

@syncer.enqueue_updates([@theme["snippets/snippet.liquid"]])
@syncer.wait!
end

private

def serve(response_body = "", path: "/", headers: {})
app = lambda do |_env|
[200, headers, [response_body]]
end
stack = HotReload.new(@ctx, app, theme: @theme, watcher: @watcher)
stack = HotReload.new(@ctx, app, theme: @theme, watcher: @watcher, syncer: @syncer)
request = Rack::MockRequest.new(stack)
request.get(path).body
end
Expand Down
2 changes: 1 addition & 1 deletion test/shopify-cli/theme/dev_server/integration_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def test_streams_hot_reload_events
file = Pathname.new("#{ShopifyCLI::ROOT}/test/fixtures/theme/assets/theme.css")
file.write("modified")
begin
assert_equal("2a\r\ndata: {\"modified\":[\"assets/theme.css\"]}\n\n\n\r\n", socket.readpartial(1024))
assert_includes(socket.readpartial(1024), "2a\r\ndata: {\"modified\":[\"assets/theme.css\"]}\n\n\n\r\n")
ensure
file.write("")
end
Expand Down