Bug when Vite HMR

[levchak0910]

This is a fix report and an enhancement and a plugin request all in one, so I decided to create a blank issue

I have encountered the same issue as here #24 I have seen your fix #25 and would say I am not really agree with meaning of fix

My concerns:

Code:

1. This fix is more of a breaking change than a fix.
2. It is not very good to have different behaviour depending on some variable from outside. What if (suddenly :confused:) that variable will not be provided (or will, but incorrectly) from outside
3. This error still happens in SSR mode. Vite creates commonjs bundles, so when someone is working with HMR using vite devServer in SSR mode, your package will be bundled and __DEV__ condition will look like this

Steps to reproduce the SSR bug:

1. clone https://github.com/levchak0910/vue3-ssr-realworld-example-app
2. checkout to branch: harlem-bug
3. yarn dev
4. update something in file /src/store/user.ts
5. see bug

Or one another is here

DX:

1. People, who see this warning and don't know the project's codebase, will not understand what is going on and why they see that warning, meanwhile they didn't write anything irrelevant
2. People won't understand whether they wrote somewhere really duplicate store or it is just HMR
3. Inconsistent behavior for dev and prod env. People write the code and don't see (or don't understand) this warning and everything works OK. But when they push code to server, create a build, and deliver the code to users, there will be thrown a real error instead replacing the store

Possible solution

On my opinion handling vite's specific bundling process should not be a part of this library, but instead could be handled by a plugin

I would suggest to do 2 things:

1. Create new event (by eventEmitter) called store:beforeCreate (or whatever) and dispatch that before creating a store (obviously :smile:), which will provide name of creating store and writable stores map
2. Create new plugin called harlem-plugin-vite. In this plugin we will listen event store:beforeCreate and delete creating store from writable stores map before creating, so function createStore will be able to work as intended

By this solution we will have:

1. consistant behavior in dev and prod env
2. no breaking changes
3. no confusing warnings

What do you think about all of it? If you agree I can write all this staff and make pull request

[levchak0910]

Hi @andrewcourtice Any updates on this?

[yaquawa]

Running into the same issue, it really shouldn't use a global store in the SSR mode, it's not a issue of Vite, it could be problematic on a node server too, because on the server side a Node.js server is a long-running process, the solution should be a part of https://github.com/andrewcourtice/harlem/tree/main/plugins/ssr I think.

[levchak0910]

@yaquawa, you are right, SSR could be implemented not only by Vite

upd: But this bug was also caused by HMR in normal CSR (#24), so I think plugin/ssr is also not a good fit to fix

[yaquawa]

@levchak0910 I think that can be fixed by calling the viteDevServer.moduleGraph.invalidateAll() Anyway, it works with ssr-glue/vite-plugin on SPA mode in my environment. Perhaps you are mixing the client entry and server entry together, which could cause the issue.

[andrewcourtice]

@levchak0910 Apologies for taking a while to get back to you.

On reflection, I agree with you that my solution in #25 is not ideal. My intention going forward is to emulate the error handling conventions of Vue which is to warn in development and either do nothing or throw an error (depending on the context) in production.

In this instance it makes sense to do nothing in production and allow store items to be overwritten. It's also worth noting that my solution in #25 leans heavily on node shimming which I am keen to move away from.

That being said, @yaquawa 's solution is ideal - with one minor change. I intend to include an options parameter in the createStore method which include's the allowOverwrite option but also caters for more options in the future should they be necessary.

[yaquawa]

@andrewcourtice Thanks for accepting the PR :) For some reason, I couldn't build the source, not sure why.. but, just for you information, you may want to give tsup a try, powered by esbuild too, it has greatly reduced my cost of writing build scripts manually.

[andrewcourtice]

@yaquawa Thanks for your contribution - I really appreciate it 😄

Let me know how you go with version 1.2.4.

Also, thanks for suggesting tsup - It looks great! I'll have to give it a go.

Closing this issue as it is fixed in v1.2.4

[yaquawa]

@andrewcourtice Thanks ! I could confirm that it works on my environment now!