Force internal port to 8080 #2403
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
infomiho
reviewed
Dec 17, 2024
| 'internal_port', | ||
| )} in the fly.toml. | ||
|
|
||
| This means your server app might not be accessible. |
There was a problem hiding this comment.
I got confused by this part, what are we trying to say here: if the internal_port option is not present in the Fly TOML file the server app might not work correctly because Fly won't know how to reach it?
|
|
||
| Press any key to continue or Ctrl+C to cancel.`); | ||
| } else { | ||
| // the default fly.toml assumes port 8080 (or 3000, depending on flyctl version). |
There was a problem hiding this comment.
I feel like this comment is redundant - we are setting the value to something we want, the default is not really important to know for the reader.
There was a problem hiding this comment.
In this file, here:
we are also setting the server env var to8080 and we want these two to change at the same time. So, I'd extract the port information into a variable and use it in both places e.g. serverPort or similar.
| @@ -85,6 +85,7 @@ async function setupServer(deploymentInfo: DeploymentInfo<SetupOptions>) { | |||
| await $`flyctl launch --no-deploy ${launchArgs}`; | |||
|
|
|||
| const minMachinesOptionRegex = /min_machines_running = 0/g; | |||
| const internalPortOptionRegex = /internal_port = \d+/g; | |||
There was a problem hiding this comment.
We already defined this regex in the setupClient fn, and since it's the same thing, I'd extract this regex to the top and use it both setupServer and setupClient functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a proposed fix for #2398. This is similar to a fix that was made to the client setup. Another option would be to change like 120 to
PORT=3000but my guess is older version offlyctlused port 8080 and don't want to break those.Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following:
Update example apps if needed
If you did code changes and added a new feature or modified an existing feature, make sure you satisfy the following:
waspc/examples/todoAppas needed (updated modified feature or added new feature) and manually checked it works correctly.waspc/headless-test/examples/todoAppand its e2e tests as needed (updated modified feature and its tests or added new feature and new tests for it).