fix: check if data exists in the hot object

[aminya]

Fixes #29

[ryansolid]

@katywings I'm gathering this is related to the new approach using hot.data. Does this fix make sense or does it have other implications?

[katywings]

@ryansolid Yes, this fix is related to the new approach I implemented in b58ea031720fef3b0db20f30d644d962847cae6c. When a few people mentioned the error #29 in Discord, I asked @lxsmnsyc if he's interested in a fix. I didn't get a conclusive answer so far, maybe he can answer directly here.

From my side: I think this fix is very reasonable. When I implemented the new approach, I didn't know that there are environments which set hot but don't set hot.data ;).

About the implications, I can see the following:

1. Some people using Vitest and vite-plugin-solid with the default hot: true might assume, that solid does HMR in their tests and they will not know that it actually isn't hot. Personally I wouldn't do it in this case, but I think some people might like to be informed with a console.info/warn when HMR is disabled for some reason. It could be a warning like vite-plugin-solid: Vitest doesn't support HMR. Please consider manually setting hot: false in the plugin config.
2. In addition to 1. - If we dynamically disable HMR on the basis of hot.data without notice and the user writes tests on this basis, what happens in the future if vitest actually adds hot.data? There is the rather unlikely chance that such a scenario might break some user tests expecting HMR to not work 😅.

Edit: If you are interested in notifying the users: Vitest sets the env variable process.env.VITEST which we could use as a way to check if the environment is Vitest.

[lxsmnsyc]

@katywings I was inconclusive because the issue happened in Vitest, which I don't think should have the HMR on 😅 but I'll leave the decision to you

[matt-way]

I'm not sure this is the correct fix. Shouldn't it be something like:

if (hot) {
    const refreshData = hot?.data?.[HOT_DATA_PREFIX]  || {};
    if (refreshData[id]) {

This still allows systems to function that have set hot but haven't implemented the default empty .data object.

[katywings]

@matt-way Yes and no: without the hot.data HMR works inconsistently. Essentially on systems without hot.data the following bug would happen with your approach: https://github.com/solidjs/solid-refresh/issues/26.

[aminya]

Could you merge this for now and decide on future improvements later? This bug prevents me from running the tests.

[katywings]

@ryansolid @lxsmnsyc Maybe its a misunderstanding but just so that you know: I don't have account permissions to merge this 😅.

[lxsmnsyc]

@katywings I don't have it either, I'm just here to review PRs 🤣

[quantuminformation]

legends