Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watch, hot reload, server restart on 1.1.0 #31

Closed
brunoimbrizi opened this issue Jan 18, 2023 · 8 comments · Fixed by #32
Closed

Watch, hot reload, server restart on 1.1.0 #31

brunoimbrizi opened this issue Jan 18, 2023 · 8 comments · Fixed by #32
Assignees
Labels
enhancement New feature or request needs help Accepting pull requests for improvements

Comments

@brunoimbrizi
Copy link
Contributor

I noticed some issues on version 1.1.0 and I also have a few questions:

  1. Is config() still needed?
  2. Passing watch = false on initialisation now has no effect.
  3. Is it necessary to call server.restart() in handleHotUpdate()?
    Restarting the server makes every reload much slower. A quick test removing that call (or removing handleHotUpdate altogether) resulted in much faster reloads. It seems that Vite already understands the project needs to be rebuilt without having to force restart the server.
@UstymUkhman
Copy link
Owner

Hi @brunoimbrizi, thanks for using this plugin and filling this issue to improve it!

  1. Nope, not anymore, I will remove it, thanks!
  2. Oops, I've forgot about this option. Will fix, thx!
  3. Now I'm not sure about it anymore. It was added because of this issue. And then handleHotUpdate was added because of this comment. Maybe it should use handleHotUpdate but without server.restart()..? I'm still trying to understand the best way to trigger vite's HMR, so any hint from your side would be much appreciated, thanks!

@UstymUkhman UstymUkhman self-assigned this Jan 18, 2023
@UstymUkhman UstymUkhman added bug Something isn't working needs help Accepting pull requests for improvements labels Jan 18, 2023
@brunoimbrizi
Copy link
Contributor Author

@UstymUkhman I submitted a PR removing config (1), skipping the hot update when !watch (2) and leaving handleHotUpdate to Vite's default behaviour which is to replace affected modules - so no more server.restart() (3)

Regarding a better approach for HMR, ideally it should be possible to update the shader without reloading the page. I can think of a way to do that with Three.js, by replacing the shader string directly in the materials that use it and forcing a shader recompile by setting material.needsUpdate = true. This would be very opinionated and I don't know if it could be adapted to other 3D libraries (or no libraries), but if you think something like this would be interesting I could work on it and submit a proposal.

@UstymUkhman
Copy link
Owner

Thank you so much @brunoimbrizi! I'm gonna merge your PR, check everything again with astro and plain vite and update you here abot the progress.

What I'm pretty sure about is that with configureServer before and with handleHotUpdate now, the page does not actually gets reloaded and shaders are updated under the hood when used with and without three.js, so there's a good chance everything should work fine just with your updates. As I've said, need to do a bit more research on this. Thanks again! ❤️

@UstymUkhman
Copy link
Owner

Ok, v1.1.1 was just released, so I'm closing this one with some info:

  • Unfortunately this update does not work when changing a chunk that is not directly imported into a JavaScript file. I'm talking about chunks added by using the #include directive within some shader, so the only way I've found to fix this is to reload vite's dev server.

  • When support for hot reloading was introduced for the first time in v0.5.0, I was changing vite.config file timestamp to trigger server restart. Since Astro does not have a vite.config file, this caused this issue, so I switched to server.restart() in configureServer function when the watch option was enabled.

  • Then I saw this comment and switched to handleHotUpdate. Even if I haven't saw any "weird blank page between page reloads", by reading the docs, I thought it was the right thing to do.

  • And here is where things get more interesting. If using configureServer and spamming Ctrl + S (as mentioned here), the dev server is slightly more stable and crashes a bit less often compared to handleHotUpdate. And when it does, most of the time you can reload it just by saving the file once again. With handleHotUpdate I've found myself to have manually reload the page in order for shader changes to take effect, more than I had to when using configureServer.

  • Finally, when testing the same thing with Astro, I haven't see any difference between configureServer and handleHotUpdate (again no "weird blank page between page reloads"), both were working just fine. Btw, Astro is reloading the sever automatically whenever it crashes in both cases. So in v1.1.1 I decided to stick with configureServer. If there is any issue with this or some better option to both or a strong motivation to prefer handleHotUpdate, please let me know and I will consider a rollback to handleHotUpdate. Thanks!

@brunoimbrizi
Copy link
Contributor Author

brunoimbrizi commented Jan 25, 2023

Hi @UstymUkhman, my main motivation to use handleHotUpdate is how much faster the page is reloaded. In my current project every time I change something in a shader file it takes 13s to refresh the page with server.restart() as opposed to <1s without it. I also tested it on a smaller project and the server restart happens much faster, so I understand how this might not be a problem in many use cases, but on a medium-large project it does become a problem.

Unfortunately this update does not work when changing a chunk that is not directly imported into a JavaScript file.

True. I only noticed it after I submitted the PR.
But I still think there is a way to use handleHotUpdate, as long as the included chunks are added to the ModuleGraph.
I don't have a full solution yet, but I'd like to use this thread to bring an idea up.

Assume the following scenario: App.js imports AAA.glsl which includes BBB.glsl

// App.js
import fragmentShader from './AAA.glsl'
// AAA.glsl
#include ./BBB.glsl
// BBB.glsl
vec3 getColor() { return vec3(0.0, 0.0, 1.0) }

Then in order to see what's going on with handleHotUpdate, we add clearScreen: false to vite.config.js.
And in vite-plugin-glsl/src/index.js, we add:

handleHotUpdate ({ file, modules }) {
  console.log('handleHotUpdate', file, modules);
}

After making a change to AAA.glsl, the page reloads almost immediately and the output in the terminal is:

handleHotUpdate C:/.../AAA.glsl [ ModuleNode ]

After making a change to BBB.glsl, the page doesn't reload and the output in the terminal is:

handleHotUpdate C:/.../BBB.glsl []

So a change to BBB.glsl does trigger handleHotUpdate even if it's never imported directly by any JavaScript file, but it doesn't trigger a page reload because it's not part of Vite's ModuleGraph.

This is a similar scenario to a CSS @import and I think we can take inspiration from how Vite handles that in the built-in plugins/css.ts using createFileOnlyEntry.

  // some deps, like a css file referenced via @import, don't have its own
  // url because they are inlined into the main css import. But they still
  // need to be represented in the module graph so that they can trigger
  // hmr in the importing css file.
  createFileOnlyEntry(file: string): ModuleNode { }

I think something like that could be implemented for GLSL #include. What do you think? Is it worth pursuing the idea?

@brunoimbrizi
Copy link
Contributor Author

I got a quick example working hot reloading include-me.glsl which is not referenced by any JavaScript file. It's not a full implementation yet, just proof-of-concept. Here's a screen recording:
https://user-images.githubusercontent.com/880280/214721203-d6372b8d-a4c9-45d2-b995-e34d6320b483.mp4

@UstymUkhman
Copy link
Owner

@brunoimbrizi Looks awesome! Please feel free to submit a PR once you have a stable version and thank you so much for contributing to this project.

...my main motivation to use handleHotUpdate is how much faster the page is reloaded.

Ohh, I see. I have never tested it on large scale projects so I've never faced this issue, but sure, this is a super valid motivation.

This is a similar scenario to a CSS @import and I think we can take inspiration from how Vite handles that in the built-in plugins/css.ts using createFileOnlyEntry.

I think something like that could be implemented for GLSL #include. What do you think? Is it worth pursuing the idea?

Definitely! I have never thought it was possible to use and update vite's ModuleGraph, but since you've found an example of how CSS Plugin does just that, I would say it's the way to go for this plugin as well since at the end, the logic behind the #include directive is pretty similar to the @import rule. Thanks again and good luck with that!

@UstymUkhman UstymUkhman reopened this Jan 26, 2023
@UstymUkhman UstymUkhman added enhancement New feature or request and removed bug Something isn't working labels Jan 26, 2023
@UstymUkhman
Copy link
Owner

I'm closing this since v1.1.2 was just released. Your update that allows to leverage vite's ModuleGraph works like a charm! Thanks again for your contribution, I appreciate it a lot. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs help Accepting pull requests for improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants