gertqin / vuex-class-modules

Typescript class decorators for vuex modules
MIT License
194 stars 20 forks source link

#51 wip: Add vite hmr support #52

Closed MartB closed 3 years ago

MartB commented 3 years ago

see https://vitejs.dev/guide/api-hmr.html

bodograumann commented 3 years ago

Thanks for giving this a try. I’m not really familiar with vite myself unfortunately. You should definitely try to get this working locally first with npm link or the like.

Afaict this does not work as you propose. tsc complains:

src/module-factory.ts:173:25 - error TS1343: The 'import.meta' meta-property is only allowed when the '--module' option is 'esnext' or 'system'.

173       if (module.hot || import.meta.hot) {
                            ~~~~~~~~~~~

src/module-factory.ts:173:37 - error TS2339: Property 'hot' does not exist on type 'ImportMeta'.

173       if (module.hot || import.meta.hot) {

As I understand it, import.meta.hot is always present in vite developement mode and it lets you handle hmr events.

MartB commented 3 years ago

Thanks for giving this a try. I’m not really familiar with vite myself unfortunately. You should definitely try to get this working locally first with npm link or the like.

Afaict this does not work as you propose. tsc complains:

src/module-factory.ts:173:25 - error TS1343: The 'import.meta' meta-property is only allowed when the '--module' option is 'esnext' or 'system'.

173       if (module.hot || import.meta.hot) {
                            ~~~~~~~~~~~

src/module-factory.ts:173:37 - error TS2339: Property 'hot' does not exist on type 'ImportMeta'.

173       if (module.hot || import.meta.hot) {

As I understand it, import.meta.hot is always present in vite developement mode and it lets you handle hmr events.

Yeah, im facing a similar typing issue with my own apps. I just wanted to test this quickly before leaving for a walk, but yeah i figured the linter would complain.

Im not sure where to go from here i can only say that hot-reloading is working correctly with the patch (when ignoring the type issue). Do you have any idea how to mitigate this?

The esnext thing is probably a linter config option one could set, not sure if it has any downsides for this project. The .hot thing is sadly annoying, maybe its possible to check this in a smarter less error prone way?

gertqin commented 3 years ago

As commented on the issue, I quickly tried to add the vite types, but then I got other type errors. Maybe we can just add a vite-hmr.d.ts file and define import.meta.hot? But we should probably also check that import.meta exists, otherwise we might get js errors when using it with webpack, i.e. || import?.meta?.hot.

@MartB if you are up for it, can you try this and update your pull request?

MartB commented 3 years ago

@gertqin im a total newbie when it comes to maintaining packages, so yeah i have no idea how to satisfy all needs here.

The thing i pushed gets rid of the tsc warnings but im not sure if thats the right approach, feel free to work on your own take here.

Edit: This does not seem like the right approach, this would break tests as they dont support esnext aswell, meh. I guess you/someone else would need to mock the import.meta.hot types.

gertqin commented 3 years ago

Thanks for the effort. Indeed, supporting import.meta proved to be very difficult as jest doesn't support it, and I dont like having to change build target just to support a dev feature. I've come up with another solution 7cc6e9c80f1d4416b328fdfb1fc4bd558d162587, which isn't very clean, but at least should be enough for now.