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

Use HMR port when specified #63

Merged
merged 4 commits into from
Jul 22, 2022
Merged

Use HMR port when specified #63

merged 4 commits into from
Jul 22, 2022

Conversation

guilheb
Copy link
Contributor

@guilheb guilheb commented Jul 6, 2022

This PR updates the hot file generation to use Vite's server.hmr.port config option when specified by the user.

This configuration option is intended to tell the Vite client where to find the HMR server port for cases where it is different from server.port. I believe it makes sense for us to use it as well.

This probably helps a range of use cases, but specifically, it allows users to run Vite inside a Sail container that is running inside WSL, where the Vite dev server needs to be configured on the container's public address, but where the host machine cannot connect to this same address.

Users running Vite inside Sail inside WSL will need to add the following to the defineConfig section of their vite.config.js file:

server: {
    hmr: {
        port: 3001,
    },
},

@guilheb guilheb mentioned this pull request Jul 6, 2022
@jessarcher
Copy link
Member

Have you tried configuring the VITE_PORT env var? Current versions of Sail ship with a port mapping and I believe this PR would cause people to break that link.

@jessarcher
Copy link
Member

By the way, the TypeScript error with your PR is that config.server.hmr.port can be undefined even when config.server.hmr is an object.

@guilheb
Copy link
Contributor Author

guilheb commented Jul 6, 2022

I did not try the VITE_PORT var, as I am not using Sail. I simply used the same wording as pull request #42 (referencing Sail), which is very similar.

My setup is actually using custom Docker containers. Since I have multiple containers running on my machine, I need a different port to access each Vite server. I have Vite running on the default port (3000) inside the containers, but each container has a different port mapped to 3000. For example, on a particular container, I mapped port 3001 (external) to 3000 (internal). Then on another one, 3002 to 3000. Etc.

Sorry for the poor pull request, it’s my first pull request ever, in a language I’m not familiar with, and I’m not equipped to test different setups than mine.

@guilheb
Copy link
Contributor Author

guilheb commented Jul 6, 2022

By the way, the TypeScript error with your PR is that config.server.hmr.port can be undefined even when config.server.hmr is an object.

That is weird, I followed the same syntax as lines 394 and 399.

@jessarcher
Copy link
Member

That is weird, I followed the same syntax as lines 394 and 399.

The difference with those is they are later checked for undefined/null and another value is used instead that is guaranteed not to be undefined.

You might be better off with something like this:

     const configHmrPort = typeof config.server.hmr === 'object' ? config.server.hmr.port : null;
     const port = configHmrPort ?? address.port

@jessarcher
Copy link
Member

Sorry for the poor pull request, it’s my first pull request ever, in a language I’m not familiar with, and I’m not equipped to test different setups than mine.

No worries!

I would suggest adding - '${VITE_PORT:-5173}:${VITE_PORT:-5173}' to the ports section of each container that is running Vite. See Sail for an example of what that looks like.

Then you can configure the VITE_PORT environment variable for each container and it will use that value for the port mapping, and also to configure Vite and the hot file to use that custom port.

@guilheb
Copy link
Contributor Author

guilheb commented Jul 6, 2022

When I look at index.ts line 114, it seems that VITE_PORT is only used when LARAVEL_SAIL is set. In my case, I'm not using Sail, so it's not set.

@jessarcher
Copy link
Member

When I look at index.ts line 114, it seems that VITE_PORT is only used when LARAVEL_SAIL is set. In my case, I'm not using Sail, so it's not set.

Oh, good point! You could try setting that variable as well, although I'm not 100% sure what else it's used for.

As for this PR, I'll need to have more of a think about the impacts of this with the VITE_PORT env var for Sail users.

@jessarcher jessarcher marked this pull request as draft July 11, 2022 00:49
@enjibby
Copy link

enjibby commented Jul 21, 2022

Could I also suggest investigating server.hmr.clientPort as that is a variable specifically designed to handle as a setting for client-side access, regardless of the server-side port on which vite is listening.

@enjibby
Copy link

enjibby commented Jul 21, 2022

I have a team of developers that are all set up to use Docker stacks and Traefik as a reverse-proxy/ingress container on each of their machines as we have a number of projects in the pipe at any one time. Now that Vite is the js-builder of choice in new Laravel projects, and it seems set on making the HMR server start as part of dev, I need to configure new projects to work with HMR by default (without having to co-ordinate which projects use which ports for which listener all the time).

In my test case, I clearly have the vite-hmr server accessible through ingress, as I can hit e.g. https://test5-vite.nb.test/@vite/client in a broswer and I get a HTML response "This is the Vite development server that provides Hot Module Replacement for your Laravel application. To access your Laravel application, you will need to run a local development server." and npm run dev outputs:

  VITE v3.0.2  ready in 373 ms

  ➜  Local:   https://localhost:3220/
  ➜  Network: https://172.18.0.5:3220/
  ➜  Network: https://172.20.0.5:3220/

  LARAVEL v9.21.3  plugin v0.5.0

  ➜  APP_URL: https://test5.nb.test

The Vite server should not be accessed directly. Your Laravel application's configured APP_URL is: https://test5.nb.test

I have the following in my vite.config.js:

    server:{
        host: '0.0.0.0',
        strictPort: true,
        port: 3220,
        https: true,
        open: false,
        hmr: {
            host: 'test5-vite.nb.test',
            port: 3220,
            clientPort: 443
        }
    }

FTR I have chosen port 3220 myself at random (I was finding 3000 at the time was conflicting with other services in the same container) but I am not exposing this outside the container (to avoid conflicts) and instead creating some docker labels to advise Traefik as such:

      - "traefik.http.routers.${COMPOSE_PROJECT_NAME}-vite-sslweb.rule=Host(`${COMPOSE_PROJECT_NAME}-vite.${REVPROXY_TLD}`)"
      - "traefik.http.routers.${COMPOSE_PROJECT_NAME}-vite-sslweb.entrypoints=https"
      - "traefik.http.routers.${COMPOSE_PROJECT_NAME}-vite-sslweb.tls=true"
      - "traefik.http.routers.${COMPOSE_PROJECT_NAME}-vite-sslweb.service=${COMPOSE_PROJECT_NAME}-vite-sslweb"

      - "traefik.http.services.${COMPOSE_PROJECT_NAME}-vite-sslweb.loadbalancer.server.scheme=https"
      - "traefik.http.services.${COMPOSE_PROJECT_NAME}-vite-sslweb.loadbalancer.server.port=3220"

I have also previously had success configuring apache or nginx to ProxyPass a subfolder to a websocket port, but that is outside the bounds of this discussion.

My problems start when I hit the laravel project as it fails while trying to websocket-connect to https://test5-vite.nb.test:3220/@vite/client.

Setting LARAVEL_SAIL is a no-go, mostly because I'm simply not using Laravel Sail or set-up in any Laravel Sail type way.

Is there any reason why the laravel-vite-plugin should not respect the configuration I have provided?

@jessarcher jessarcher merged commit 6ed250d into laravel:main Jul 22, 2022
@enjibby
Copy link

enjibby commented Jul 22, 2022

Thank you so much @jessarcher !

@guilheb
Copy link
Contributor Author

guilheb commented Jul 22, 2022

Yay, my first pull request ever!

@timacdonald
Copy link
Member

Congrats @guilheb! Thanks so much for helping to improve the ecosystem for everyone ❤️

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 this pull request may close these issues.

4 participants