dario-piotrowicz / vite-environment-6.0.0-alpha-experimentations

1 stars 1 forks source link

HMR improvements #14

Closed jamesopstad closed 1 month ago

jamesopstad commented 1 month ago

This fixes various bugs in the HMR implementation and adds some more logs to validate that the HMR is working.

jamesopstad commented 1 month ago

This PR resolve https://github.com/dario-piotrowicz/vite-environment-6.0.0-alpha-experimentations/issues/10 for the workerd and node-vm environment providers. I started adding the changes to the node-process environment but encountered some problems with buffering of events of stdout that need resolving separately.

dario-piotrowicz commented 1 month ago

This PR resolve #10 for the workerd and node-vm environment providers. I started adding the changes to the node-process environment but encountered some problems with buffering of events of stdout that need resolving separately.

Regarding the PR resolving #10, this PR is mostly adding checks for custom events (and hot.accept as we had before), should we also look into adding checks for hot.dispose, hot.prune, hot.data, hot.decline, etc... (from https://vitejs.dev/guide/api-hmr.html)

Or do you think that the current API tests give us enough confidence that HMR is working reasonably well?

dario-piotrowicz commented 1 month ago

This PR resolve #10 for the workerd and node-vm environment providers. I started adding the changes to the node-process environment but encountered some problems with buffering of events of stdout that need resolving separately.

Regarding the PR resolving #10, this PR is mostly adding checks for custom events (and hot.accept as we had before), should we also look into adding checks for hot.dispose, hot.prune, hot.data, hot.decline, etc... (from https://vitejs.dev/guide/api-hmr.html)

Or do you think that the current API tests give us enough confidence that HMR is working reasonably well?

I am also wondering if we should invest the time and do some sort of automated testing that checks for the App's functionality alongside HMR 🤔 (so that we can be sure that we don't break things as we go along and update packages)

I think it'd be very much worth it... but given the tight schedule we have I am not sure it it makes complete sense... any thoughts?

jamesopstad commented 1 month ago

Regarding the PR resolving #10, this PR is mostly adding checks for custom events (and hot.accept as we had before), should we also look into adding checks for hot.dispose, hot.prune, hot.data, hot.decline, etc... (from https://vitejs.dev/guide/api-hmr.html)

Or do you think that the current API tests give us enough confidence that HMR is working reasonably well?

I've included the minimum number of methods from the API that we needed to confirm that the HotChannel is functioning correctly and both sending and receiving events. Adding hot.data would be straightforward. I think hot.dispose and hot.prune would require making the example framework code a bit more complicated so modules get added and removed.

I am also wondering if we should invest the time and do some sort of automated testing that checks for the App's functionality alongside HMR 🤔 (so that we can be sure that we don't break things as we go along and update packages)

I think it'd be very much worth it... but given the tight schedule we have I am not sure it it makes complete sense... any thoughts?

Do you have some thoughts on how we could automate this?

dario-piotrowicz commented 1 month ago

I've included the minimum number of methods from the API that we needed to confirm that the HotChannel is functioning correctly and both sending and receiving events. Adding hot.data would be straightforward. I think hot.dispose and hot.prune would require making the example framework code a bit more complicated so modules get added and removed.

yes that's what I was thinking... adding those might be a bit cumbersome... it might or not be worth it... maybe not... (not at this point at least) 🤔

Do you have some thoughts on how we could automate this?

No sorry 😓, the only think I am thinking is a combination of some e2e tool such as playwright for checking the HTML displayed in the browser and some nodejs process testing for the server side logs... although it might be quite tricky to implement as it would also require file system files updates and whatnot....

jamesopstad commented 1 month ago

I've just added a log for import.meta.hot.data. I think the subset of HMR methods we already have should be enough to confirm that everything is working though.