-
Notifications
You must be signed in to change notification settings - Fork 20
fix: changed the Vite port to the new Vite default port 5173 #792
Conversation
I don't think we have a way of handling different versions, do we? Presumably this will cause the reverse problem for older versions of Vite. I guess that's less of a problem because new projects are more likely to use new Vite, but it's worth noting. |
de3fe87
to
90f4a10
Compare
90f4a10
to
632ecd5
Compare
@ascorbic, yeah it's a good point. Have we handled something like this in the past? I'm not sure how problematic it will be. We can definitely make support aware of the issue. Maybe a notice in the CLI of the port upgrade via a new field in the framework info JSON file if it fails via timeout to find the server? {
"id": "vite",
"name": "Vite",
"category": "build_tool",
...
"dev": {
"command": "vite",
"port": 5173,
"pollingStrategies": [{ "name": "TCP" }]
"migrationMessage": "Vite's default port is now 5173. See the migration guide https://vitejs.dev/guide/migration.html#architecture-changes-and-legacy-options"
},
...
} We could also check for both ports, but that seems more of bandaid solution. |
There is an env var field available, so maybe we can use one to force the old version to use the new port number (or vice versa). Worth a try. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find, LGTM 🚀
src/frameworks/qwik.json
Outdated
@@ -9,7 +9,7 @@ | |||
}, | |||
"dev": { | |||
"command": "vite", | |||
"port": 3000, | |||
"port": 5173, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this one @ascorbic, it looks like Qwik has set the port to 5173 if you create a new Qwik app. Since this project is so new, I imagine for this config change it's OK?
@@ -9,7 +9,7 @@ | |||
}, | |||
"dev": { | |||
"command": "vite", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ascorbic, I thought about this one some more, and since this is the default if they have nothing configured, we can explicitly set it to "command": "vite --port 5173",
? That way anyone who was using it previously won't run into any issues and new folks won't run into issues either.
If we were to use an environment variable, we'd have to use a vite config to consume it to set the port. The only problem with this is there's a disconnect between the command and the port fields, although this is a minor issue. We could also remove the --port 5173
at some point since most people will probably have updated at some point.
Thoughts?
I thought about this again and explicitly adding the port could be more problematic. I think just tearing the bandaid off and notifying folks in support as well as Discords about the port change might be a better way. And again, the CLI will mention to configure targetPort
in your netlify.toml if it's waiting.
➜ vite-broken-with-ntl
dev/issues/vite-broken-with-ntl on main [!?] via v16.15.1
➜ ntl dev
◈ Netlify Dev ◈
◈ Ignored general context env var: LANG (defined in process)
◈ Starting Netlify Dev with Vite
> vite-broken-with-ntl@0.0.0 dev
> vite
VITE v3.0.2 ready in 195 ms
➜ Local: http://127.0.0.1:5173/
➜ Network: use --host to expose
⠙ Waiting for framework port 3000. This can be configured using the 'targetPort' property in the netlify.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this message with SvelteKit first changed and was not immediately aware that it couldn't find a port. It became clear eventually, but this message is stalled out. Is there a way to error out if it doesn't find one in a specific amount of time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @brittneypostma , erroring out after a specified amount of time would be useful so it removes the perception of 'hanging' and a useful error message can help the user troubleshoot a bit better with respect to the expected port for the framework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the timeout makes sense. Just wondering what makes sense in terms of time taking into account not everyone is running an M1. Maybe 30 seconds and make the message clearer? And if it fails to find it, we can add an error message like you suggested @ericapisani.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 30 seconds and make the message clearer?
I'm not sure if this is too aggressive, but I wonder if we would want to make the timeout a bit shorter (10-15 seconds) as I imagine that folks will ctrl/cmd+c the process sooner in the 30 second time frame 🤔 I'm not sure if we have any data to provide some guidance on this for us though so we can make a more informed decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 10-15 seconds may be too aggressive given that not everyone is running an M1. I think if we do 30 and make the message clearer about the port we should be good alongside the port change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on @seldo's comments in Slack, i.e. "People are overwhelmingly on version 3", I think it's safe to flick the switch to port 5173 and folks on older versions will get the port warning the CLI provides.
src/frameworks/svelte-kit.json
Outdated
@@ -9,7 +9,7 @@ | |||
}, | |||
"dev": { | |||
"command": "svelte-kit dev", | |||
"port": 3000, | |||
"port": 5173, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ascorbic, for svelte-kit, I know @brittneypostma ran into this the other day, so I wonder if it's OK to do the port upgrade to the new vite default port and for folks who may be using the old port, just update it? The error should be fairly clear when the CLI times out waiting for a server that is not running on port 3000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this comment after my other one, so there is a timeout I'm just too impatient to wait for it 😅
✅ Deploy Preview for framework-info ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #791
Vite changed the default port for from
3000
to5173
on May 31st or around that time. Seehttps://github.com/vitejs/vite/blob/2826303bd253e20df2746f84f6a7c06cb5cf3d6b/docs/guide/migration.md#dev-server-changes
vitejs/vite#8417
For us to review and ship your PR efficiently, please perform the following steps:
ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a
typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)