Shopify / shopify-cli

Shopify CLI helps you build against the Shopify platform faster.
https://shopify.dev/tools/cli
Other
1.01k stars 202 forks source link

Hot reload not working correctly #1409

Closed tommypepsi closed 2 years ago

tommypepsi commented 3 years ago

Issue summary

The hot reload seems to be refreshing the page but the change don't appear until the page is refreshed manually.

Expected behavior

The page should refresh when a file is changed and the change should appear immediately.

Actual behavior

The page refresh but the change is not there.

Steps to reproduce the problem

  1. Do shopify theme serve
  2. Modify file
  3. Save

Specifications

tommypepsi commented 3 years ago

Seems to depend on the file. Updating a shopify section seems to take more time(If I refresh immediately after the automatic refresh it's still not there, have to refresh another time to see the change), while updating a js file seems to be instant with the automatic refresh.

james0r commented 3 years ago

This URL hot reloads local changes to CSS and sections, allowing you to preview changes in real time using the store's data. This preview is available only in Google Chrome.

This really could be elaborated on. Which files? What tech is this using? are there options for this functionality?

Also the docs say for CSS and Sections but it appears that Templates are also being watched and updated. There should be an exhaustive list of what is and isn't watched.

lesterdefreitas commented 3 years ago

I am having similar issues, posted this issue a while back.

sergejcodes commented 3 years ago

For anyone who's interested here's the hot-reload.js file that's being appended to the body during development when using the localhost URL

Some basic takeaways:

  1. The script is using the SSE API to listen for messages sent by the Shopify CLI
  2. When the theme serve command is running, the CLI watches your project directory for any file changes on your local system and sends the path of any changed file to the script.
  3. Depending on the type of file that changed it's going to do either a hot-replacement of that file or a full page-reload
    • the file has a .css extension -> hot-replacement
    • the file is inside the sections/ directory and present on the currently viewed page -> hot-replacement
    • the file is inside the sections/ directory and not present on the currently viewed page -> page-reload
    • any other type of file -> page-reload
tommypepsi commented 3 years ago

@sergejcodes Thanks for pointing in the right direction!

I think the issue we have might be coming from there:

// Assume only one file is modified at a time
var modified = data.modified[0];

Since i'm using webpack to build our project I have a feeling maybe more than one file is updated at the same time so only the first one is refreshed. Or maybe it's not updated at the same time but it's updated too fast to be all caught by the hot reload (like it refresh and while refreshing some other files are still being updated by webpack)

stuffedmotion commented 3 years ago

Facing this issue with our team as well with our webpack based theme.

When updating a liquid file, the page refreshes but the changes are not visible yet. As if it refreshes too quickly

tonegolf71 commented 3 years ago

I have the same issue, it's like the browser is autorefreshing before the change has taken effect, then when a manual (or subsequent auto) refresh is completed, the change is there.

I am not using webpack, or any other build tool, so only one file is changing here.

I am on Windows 10, running VSC and the Shopify Serve command through Ubuntu on WSL.

tommypepsi commented 3 years ago

After digging a bit more I can see this is definitely PART of the issue. I logged the modified array and I have 10 files being modified because of webpack. So it will just refresh as soon as possible because the first file is always a js file.

// Assume only one file is modified at a time
var modified = data.modified[0];

Even without webpack, I don't know much about ruby, but from what I understand the hot reload is triggered as soon as the file is modified and not when it is finished uploading: https://github.com/Shopify/shopify-cli/blob/0d5eb6141c2d21eaed3f53f5a09d5dbe27dfa557/lib/shopify-cli/theme/dev_server/watcher.rb#L17

So most of the files will probably not have the time to finish uploading before the hot reload is triggered.

The hot reload should be triggered when all files have finished uploading.

GuillermoCasanova commented 3 years ago

Can say I am also getting this issue. JS and SCSS files update immediately, but any liquid and html files require two slow refreshes to be seen. This is the same issue I'm being faced with Shopify Packer as well on another project.

Not running any extra builds on the project using only Shopify CLI though, just this package.

james0r commented 3 years ago

Can say I am also getting this issue. JS and SCSS files update immediately, but any liquid and html files require two slow refreshes to be seen. This is the same issue I'm being faced with Shopify Packer as well on another project.

Not running any extra builds on the project using only Shopify CLI though, just this package.

You're accessing the site through the proxy right? 127.0.0.1:9292 or whatever your configuration is.

And where is this file located in the project structure? Even though the docs don't specifically say snippets are watched, i'm finding that changes to layouts, snippets, and sections are hot reloading for me.

tommypepsi commented 3 years ago

Even though the docs don't specifically say snippets are watched, i'm finding that changes to layouts, snippets, and sections are hot reloading for me.

All files are watched. Sections and CSS are actually hot reloaded without page refresh(sections with the section rendering api and css by replacing the style link element) while other files will trigger a page refresh.

The issue is mostly with the other files as they need to be uploaded for it to appear but the refresh is triggered as soon as the file is modified instead of waiting for the file to be uploaded. So if the file is uploaded fast enough it's going to be ok, but with slower connections or with build tools like webpack(because multiple file are modified) the refresh happens too fast and you have to refresh again a second or two later.

stuffedmotion commented 3 years ago

Perhaps we could be provided a config setting for a refresh delay? On Aug 17, 2021, 9:42 AM -0400, Tommy Gaudreau @.***>, wrote:

Even though the docs don't specifically say snippets are watched, i'm finding that changes to layouts, snippets, and sections are hot reloading for me. All files are watched. Sections and CSS are actually hot reloaded without page refresh(sections with the section rendering api and css by replacing the style link element) while other files will trigger a page refresh. The issue is mostly with the other files as they need to be uploaded for it to appear but the refresh is triggered as soon as the file is modified instead of waiting for the file to be uploaded. So if the file is uploaded fast enough it's going to be ok, but with slower connections or with build tools like webpack(because multiple file are modified) the refresh happens too fast and you have to refresh again a second or two later. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

GuillermoCasanova commented 3 years ago

@james0r am accessing via the specified proxy.

I second @tommypepsi on the behavior I'm getting. My internet is super fast and am using no extra builds but Shopify CLI and Dawn.

CSS and JS changes are immediate. But Liquid sections, snippets, layouts all require a secondary refresh or save to see changes.

If that is the issue here that might be a good config to include here @stuffedmotion

tommypepsi commented 3 years ago

Perhaps we could be provided a config setting for a refresh delay?

We had something like that in our build tools but it's not really reliable, you either wait too long for nothing or not long enough and you still have to manually refresh. You have to find the sweet spot but it doesn't really exist and won't be the same for everyone or not even the same everyday.

I opened a PR with a good fix for this I think. I added an observer to the syncer(the thing that uploads the files) so that the hot reload can wait for the files to be finished uploading! Whatever can be dynamically hot reloaded will be and if something cannot it will wait for the file to finish uploading. I also removed the assumption that only one file can be modified at a time(I don't like assumptions) so it could fix potential issues with build tools.

stuffedmotion commented 3 years ago

@tommypepsi awesome, that sounds like the ideal solution!

JoeyPriceless commented 3 years ago

I'm also having a lot of issues with Hot Reload when using webpack in my theme. The page will reload 2-3 times with varying delays every time i update my js bundle. In total it might take 30 seconds before it's finished.

I haven't looked at Tommy's PR but I would also like to request adding a way to disable hot reload altogether.

tommypepsi commented 3 years ago

The page will reload 2-3 times with varying delays every time i update my js bundle. In total it might take 30 seconds before it's finished.

@JoeyPriceless hmm I'm not sure if my PR will help in your case. My webpack config was modifying 10 files at a time but only triggering the reload one time. What I did is that the reload will only be triggered when the upload queue becomes empty so if your webpack config is modifiying files fast enough it should only trigger one time.

pm0u commented 3 years ago

Probably the only frustration I have left with the CLI at this point, thanks for the fix @tommypepsi let's see this get resolved!

bigskillet commented 3 years ago

Have you tried ignoring your src folder(s) and letting the compiled output trigger the reload?

I added my scripts and styles source folders to a .shopifyignore file, telling the CLI to ignore my immediate changes and wait to reload once the output file has compiled to the assets folder. Seems to be working better...

https://shopify.dev/themes/tools/cli#excluding-files-from-shopify-cli

node_modules/*
scripts/*
styles/*
.gitignore
package.json
postcss.config.js
tailwind.config.js
webpack.config.js
yarn.lock
tommypepsi commented 3 years ago

@bigskillet not sure for everyone else but the way I made our tools it is that the serve command is ran in the output folder so it's only watching at the compiled files.

I've played with the hot-reload code a lot and there's definetely an issue where the hot reload is triggered as soon as any file is updated without waiting for the files to upload. That's an issue even without build tools. The PR I opened is supposed to fix that by waiting for the files to upload before reloading.

Though the PR has been opened for 23 days and still no reviewers or activity. Not sure if there's something we can/should do to have someone take a look.

bigskillet commented 3 years ago

Thanks @tommypepsi, that makes sense.

My tooling in the past has been set up like that as well, but I'm attempting to run everything out of the root now. That's the only way I can think of to use the Github integration, while maintaining a single repo. I'm tempted to drop tooling altogether and go straight CSS/JS, so there's no backfilling.

I've been using that .shopifyignore file today and it hasn't helped much. I'm still having to manually reload constantly, especially for JS changes. Bummer.

It would be nice to have a setting to turn off hot-reload entirely.

tommypepsi commented 3 years ago

@bigskillet I'm thinking of using the subtree solution mentioned in the github integration documentation. I'm currently modifying our webpack config so that we can pass an output folder parameter to our commands. So we'll be able to output our code in, let's say, "dist_prod" folder and then we'll create a subtree with this folder (which make the folder act as it's own branch). I made a small test with subtree and the github integration seems to correctly recognize the folder as it's own branch and having the correct shopify structure.

josefarrugia commented 3 years ago

I am still experiencing the same issue on my end even with a fresh new init. @tommypepsi PR request needs to be reviewed from Shopify's end so that we can get this rolling officially

bigskillet commented 3 years ago

@tommypepsi, I saw the recommended subtree option, but still haven't wrapped my head around the setup or workflow. I'd love to see your example and learn more about it. Do you mind if DM you?

GuillermoCasanova commented 3 years ago

@tommypepsi would love to see how you're using this with a Webpack setup. Not having access to Webpack's scss and import modules features is turning a pain with Dawn.

Updated to latest Shopify CLI and still have to hit save more than twice to see changes on any file except CSS since that's hot module reloaded.

jvlvl-z commented 3 years ago

@tommypepsi would love to see how you're using this with a Webpack setup. Not having access to Webpack's scss and import modules features is turning a pain with Dawn.

Updated to latest Shopify CLI and still have to hit save more than twice to see changes on any file except CSS since that's hot module reloaded.

I second this, would love to see this working correctly with a Webpack setup.

bigskillet commented 3 years ago

This is a huge issue, even if you're not using webpack. The reload doesn't work properly with .liquid files either. I have to refresh 2-3 times after an edit. I noticed that if I save a file twice, it'll show the change from the first save. It's always a save behind...

Having to reload 2-3 times after every save has made development unbearable!

tonegolf71 commented 3 years ago

This is a huge issue, even if you're not using webpack. The reload doesn't work properly with .liquid files either. I have to refresh 2-3 times after an edit. I noticed that if I save a file twice, it'll show the change from the first save. It's always a save behind...

Having to reload 2-3 times after every save has made development unbearable!

I second that, I simply went back to using Theme Kit with theme watch until this is fixed.

bigskillet commented 3 years ago

@tonegolf71, I thought about going back to Theme Kit, and even tried using Browsersync with the Shopify CLI preview link, but from what I've seen in the documentation, the new 2.0 JSON features aren't compatible with Theme Kit?

**Edit: the new JSON templates ARE compatible with Theme Kit. The message in the Theme Kit docs, "You should use Shopify CLI if you're working on Online Store 2.0 themes," seems to be more of a suggestion, rather than the rule.

tommypepsi commented 3 years ago

I solved a conflict with my PR and it seems to have automatically assigned real people to the PR, let's hope someone will actually take a look soon!

bigskillet commented 3 years ago

@tommypepsi, is it possible to integrate your pull request manually? If so, how might I do that?

modulareverything commented 3 years ago

I'm getting this issue on a clean install of Dawn. Have to refresh the page twice as others have mentioned. Certainly burst my bubble about rewriting our theme to work with this.

tommypepsi commented 3 years ago

@bigskillet I'm not sure, I actually don't know much about ruby. I'm doing my tests by modifying shopify cli directly in the gem folder. I'm sure there's a way to package the project from my PR and use that but I don't really know how 🤷‍♂️

I'm trying to do the changes so that the PR can be merged, I really hope it won't be too long before it can move forward.

macournoyer commented 3 years ago

Sorry for the delay on this. To be clear, hot reload when changing a Liquid file should work. The update in the browser should be mostly instant, and not require a manual refresh. That is the main feature provided by the serve command. I see it doesn't work for you, and that is indeed a bug.

Could this possibly be a Windows only issue. Anyone experiencing this issue and NOT on Windows?

bigskillet commented 3 years ago

@macournoyer, I'm on macOS Catalina, 10.15.7

pm0u commented 3 years ago

@macournoyer I'm on Ubuntu 18.04, although CLI has updated a couple times since I initially posted here - I can double check this is still happening for liquid/non-compiled JS files.

pm0u commented 3 years ago

confirmed this is still happening:

shopify_cli

$ shopify version
2.5.0

$ uname -a
Linux o******* 5.13.0-7614-generic #14~1631647151~20.04~930e87c-Ubuntu SMP Fri Sep 17 00:26:31 UTC  x86_64 x86_64 x86_64 GNU/Linux
modulareverything commented 3 years ago

@macournoyer Also a Mac user and as mentioned it even happens on a clean install of the Dawn theme for me

$ shopify version
2.6.4

$ uname -a
Darwin c***** 20.6.0 Darwin Kernel Version 20.6.0: Wed Jun 23 00:26:31 PDT 2021; root:xnu-7195.141.2~5/RELEASE_X86_64 x86_64
macournoyer commented 3 years ago

Can you try with: shopify theme serve --poll and see if that fixes it? It uses a simpler approach for watching over files changes. It might fix it 🤞

@pm0u any error in the dev tools console when it fails to hot reload?

pm0u commented 3 years ago

@macournoyer had to update to 2.6.6 to get the --poll option so this is now 2.6.6 with that flag enabled

Seems to have the same issue. included console this time, just font errors for shopify CDN and a sourcemap failure for the trekkie scripts.

shopify_2

edit: also used an incognito tab in case there was a browser ext in the way. I also have cache disabled when the dev tools are open

macournoyer commented 3 years ago

Damn! Thanks for all the info. I'll keep digging.

cnscorpions commented 3 years ago

@macournoyer same issue here. Just found this msg [HotReload] Could not find link for stylesheet assets/theme.css on chrome console.

pm0u commented 3 years ago

additional detail: this seems to be inconsistent or work differently across themes. Currently working on an originally 1.0 theme with a JSON template file I am developing and the hot reload on liquid sections works properly.

edit: my previous example of random stuff above {{ content_for_layout }} also hot reloads correctly on this theme.

karreiro commented 2 years ago

Hi, everyone, I believe we actually have 2 issues in the hot reload mechanism:

1) It doesn't work when users update many files "simultaneously"

This PR https://github.com/Shopify/shopify-cli/pull/1830 fixes this issue and includes a reproducer. Thank you for the comments, clues, and your PR, @tommypepsi! They were really helpful in solving this.

2) It doesn't work when the entire page gets refreshed (reported here)

I'm really trying to reproduce it, but I'm having no success on this.

@pm0u, could you please share the X-Request-Id of the request that happens when you save the file, but the page content is not updated? That would allow me to debug things deeply.

Screenshot 2021-12-07 at 17 11 30

Here's how you may get the X-Request-Id:

Kapture 2021-12-07 at 18 38 16

Thanks!

karreiro commented 2 years ago

Hi, @pm0u! Feel free to ignore my latest message. I've been able to reproduce the second issue and solve it here :)

MaxDesignFR commented 2 years ago

Did anyone encounter an issue with hot reload and document events not firing such as DOMContentLoaded?

When a section is hot reloaded, some code might not trigger because this event is not triggered in the first place.

Only solution I've found is to dispatch this event in the async refresh function in hot-reload.js file (after the html is being replaced in the doucment). Is there a better approach to this?

karreiro commented 2 years ago

Hi @MaxDesignFR,

Another alternative is using the full-page mode in the --live-reload flag.

Triggering the DOMContentLoaded event seems like an interesting approach for some projects. However, it may generate side effects (like duplicated components) on projects that handle it differently.

Eliav2 commented 2 years ago

This issue should be reopened as no actual fix was proposed in this discussion and this issue is a real problem for theme developers.

what should we really do? disable hot reload and reload the page manually each time I change code? This is not a nice developing experience.

karreiro commented 2 years ago

Hi @Eliav2,

Thank you for the comment. Could you please share more details about your context (ideally, the actual and expected behavior)? That would help a lot in identifying a possible issue with the hot-reload mechanism.

The theme serve command now supports 3 live-reload modes (https://shopify.dev/themes/tools/cli/theme-commands#optional-parameters), and the default live-reload mode actually doesn't refresh the entire page. Still, the other two modes support developers with custom tooling around the CLI.

Thanks again for the comment!

GuillermoCasanova commented 2 years ago

@Eliav2 thank you for pointing this out. Can say using the "full-page option made it so changes made in CSS files that are pulled inside of sections are now triggering a refresh and appearing.

That said, any changes made to CSS files that are pulled in a section do not trigger HMR when in HMR mode. It's a shame since HMR is so much faster and smoother than a full page reload.

Is there a way to fix this? Or is HMR not compatible with CSS files placed inside of sections (liquid files) via link tags?