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

HMR doesn't work when changed in config #122

Closed
anurag-roy opened this issue Mar 9, 2024 · 7 comments · Fixed by #135
Closed

HMR doesn't work when changed in config #122

anurag-roy opened this issue Mar 9, 2024 · 7 comments · Fixed by #135

Comments

@anurag-roy
Copy link

How to reproduce

Update the value for server.hmr.port in your vite.config.ts

import react from '@vitejs/plugin-react';
import { defineConfig } from 'vite';

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react()],
  server: {
    hmr: {
      port: 3001,
    },
  },
});

Expectation

HMR should work via the new port

Actual

HMR does not work, socket connection fails
image

Context

Just to provide some more context. I am trying to set up my own WebSocket server on the default port, like this

import express from 'express';
import ViteExpress from 'vite-express';
import { WebSocketServer } from 'ws';

const app = express();

app.get('/hello', (_, res) => {
  res.json({
    message: 'Hello from server!',
  });
});

const server = app.listen(3000, () => console.log('Server is listening...'));

const wss = new WebSocketServer({ server, path: '/ws' });
wss.on('connection', (ws) => {
  ws.on('message', (message) => {
    console.log('Received:', message);
    setInterval(() => {
      ws.send('Echo: ' + message);
    }, 1000);
  });
});

ViteExpress.bind(app, server);

So, I think this is blocking the Vite's HMR websocket server and I want to ask Vite to use another port for that. Is there any other workaround for this?

@anurag-roy
Copy link
Author

anurag-roy commented Mar 9, 2024

@szymmis

I was going through the source code trying to debug the issue, looks like the value from vite.config.ts is lost while merging the configs here.

Can you tell me what is the use of the variable isUsingViteResolvedConfig here? Shouldn't the ternary be reversed?

const config = await getViteConfig();
const isUsingViteResolvedConfig = Object.entries(config).length > 3;

const vite = await createServer(
  mergeConfig(isUsingViteResolvedConfig ? {} : config, {
    configFile: Config.viteConfigFile,
    clearScreen: false,
    appType: "custom",
    server: {
      middlewareMode: true,
      hmr: { server },
    },
  }),
);

@szymmis
Copy link
Owner

szymmis commented Mar 12, 2024

Hi @anurag-roy, I stumbled on the same problem when I wanted to use WebSocket server myself. What I did was a quick dirty hack to comment out hmr: { server } from the library code and it was working now. Can you try doing it and telling me if it works?
I was using patch-package tool and this is my patch. You can see the whole project to get the idea how it runs. I want to do this correctly but I'll need a little more time to figure out how to do this.

When it comes to answering your question, it is because how mergeConfig works and how vite-express internaly knows that Vite as a library is not available (for example when you run the app in production environment when you don't install Vite). This code isn't ideal but otherwise there would be a problem with merging object configs and also supporting running vite-express without Vite to resolve the configs.

@anurag-roy
Copy link
Author

@szymmis Can confirm, the patch you suggested does work, many thanks!

Also thanks for answering my question, makes sense now. I'll take a look at the full code to understand how it runs!

@szymmis
Copy link
Owner

szymmis commented Mar 12, 2024

The biggest problem with this code is that it lacks comments out of my laziness, and in that kind of library code there are a lot of "weird" things going on. If you have any questions about the internals feel free to ask, I'll try to answer it the best I can but even I don't remember everything about why is something written the way it is. For the most part if it's written in a weird way, it had to be written in that way 😅

I think when it comes to resolving this problem, I'll try to detect if there is a hmr.port setting in the Vite config and not set the server option. Maybe somehow it isn't needed anyway but I have a feeling that it might break something. I will look at it as soon as I have more time.

You can also try to fiddle with it if you want, any help is greatly appreciated!

@elturpin
Copy link
Contributor

hey,

I have say "blame on me" for that issue :(

I wrote this code to use the given express server as the HMR server for vite. Otherwise, when using https, hmr would not work at all (#100). I did not realise that one could use the given server for some web socket usage and ask vite to use another port for its hmr.

Checking the the implementation of mergeConfig from ViteJs, it does not make a spacial case for hmr server. It means that you can give for the config a certain port and a server that use a different port, and vite would not complain. It should be an error case if someone sets both server and port, shouldn't it ?

@szymmis szymmis linked a pull request Jun 8, 2024 that will close this issue
@szymmis
Copy link
Owner

szymmis commented Jun 8, 2024

Hi @anurag-roy. It's been a long time, but I've finally managed to find some time and tackle this issue. This fix was embarrassingly easy to implement 😅. I hope I didn't broke anything but it seems like it works. I don't know if you are still interested in this change, but I hope you or anybody else in the future will find this useful.

@elturpin it is normal to oversee things like that. I also haven't noticed any issues before so no blame on you at all! You did a great job with contributing that change from #100.

You can check out the PR #135 if you want. I plan to release it soon in the next 0.17.0 version.

@szymmis szymmis closed this as completed Jun 8, 2024
@anurag-roy
Copy link
Author

Hey @szymmis, just tested it out and works perfectly! Thanks a ton!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants