cli icon indicating copy to clipboard operation
cli copied to clipboard

Fix an issue with hot-reloading the compiled assets (scripts.js, block-scripts.js, etc)

Open karreiro opened this issue 9 months ago β€’ 2 comments

WHY are these changes introduced?

This PR fixes an issue related to hot-reloading compiled assets (such as scripts.js, block-scripts.js, etc).

WHAT is this pull request doing?

This PR enriches the hot-reload event's payload by including details indicating whether the javascript or stylesheet tags were modified. This approach triggers a full-reload event the first time users update a section file with a javascript tag, but it caches this information, and subsequent updates will only trigger a full reload if the content of the javascript tag has changed.

How to test your changes?

  • Update your local theme-hot-reload package to use this branch
  • Run SHOPIFY_CLI_LOCAL_HOT_RELOAD=true shopify-dev theme dev
  • Change the {% javavascript %} content within a section file
  • Verify that the browser correctly performs a full refresh

Check the demo here

Post-release steps

N/A

Measuring impact

How do we know this change was effective? Please choose one:

  • [ ] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • [x] Existing analytics will cater for this addition
  • [ ] PR includes analytics changes to measure impact

Checklist

  • [x] I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • [x] I've considered possible documentation changes

karreiro avatar May 08 '25 07:05 karreiro

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
76.66% (+0.02% πŸ”Ό)
9612/12539
🟑 Branches
71.97% (+0.03% πŸ”Ό)
4755/6607
🟑 Functions 76.57% 2487/3248
🟑 Lines
77.16% (+0.02% πŸ”Ό)
9083/11771

Test suite run success

2274 tests passing in 985 suites.

Report generated by πŸ§ͺjest coverage report action from 93dc1f467f38611c890c5b9bb095033c2ddc72f3

github-actions[bot] avatar May 08 '25 07:05 github-actions[bot]

πŸ‘‹ Hey @frandiox,

Thank you for the review!

I thiiink this doesn't "warm" the cache? Meaning, it will think the tags are always "changed" the first time, right? Maybe we should integrate this cache with the themeFS? Or perhaps that's too much coupling for this code with the CLI...

It intentionally doesn't. Given that this impacts only sections containing {% javascript %} tags and that those sections inevitably require a page reload, I leaned towards accepting this trade-off of triggering an initial full refresh in the first save of those files for simplicity sake.

We would also need to check if the rest of the file (outside of the tags) is modified. The reason is, even if it says stylesheet: true, there might be another Liquid syntax change that needs a full refresh. So basically we should get {liquid: boolean; stylesheet: boolean; javascript: boolean} I think?

Actually, the mechanism to handle Liquid updates is already in place, so I don't think we need to introduce any additional logic for that scenario. Am I missing something?

Ah I see this is supposed to be a fix for section hot reloading, rather than the full feature we talked about!

100%. While this PR enriches the event payload tracking stylesheet changes β€” currently we don't take any action upon stylesheet changes.


Thanks again for the review and for setting up the foundations of this hot-reload mechanism, @frandiox -- it was much more straightforward for me to walk over the Liquid AST by reusing the approaches we have in the local proxy :)

karreiro avatar May 09 '25 12:05 karreiro

@Shopify/app-inner-loop could you please take a look at this when you have a moment?

karreiro avatar May 13 '25 07:05 karreiro

Thanks for the clarification, @frandiox! Renaming the event makes complete sense indeed :)

I've applied the change in a new commit and also released/bumped theme-hot-reload πŸš€

karreiro avatar May 13 '25 07:05 karreiro