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

feat: change default port to 4321 #7874

Merged
merged 5 commits into from
Aug 1, 2023
Merged

Conversation

ematipico
Copy link
Member

Changes

Changes Astro's defualt port to 4321

Testing

Current tests should pass

Docs

/cc @withastro/maintainers-docs for feedback!

@ematipico ematipico requested a review from a team as a code owner July 31, 2023 11:02
@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2023

🦋 Changeset detected

Latest commit: b14a807

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jul 31, 2023
@fflaten
Copy link
Contributor

fflaten commented Jul 31, 2023

Would you mind adding a line about why it's changed at all? 🙂

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a small note for the CHANGELOG. We’ll also have a bunch of places that need updating in docs: https://github.com/search?q=repo%3Awithastro%2Fdocs%20%3A3000&type=code

Have we considered what happens when the port is busy with this change? Currently, if I run astro dev, but another project is already running, Astro automatically increments the port number and uses 3001 instead (or 3002 etc. until it finds a free port). Is this now 4321, 4322, 4323?

.changeset/unlucky-sheep-build.md Outdated Show resolved Hide resolved
@ematipico
Copy link
Member Author

Would you mind adding a line about why it's changed at all? 🙂

Someone suggested the idea and it made sense for us. 4, 3, 2, 1 ... launch! Also, 3000 is often used by other frameworks.

@ematipico
Copy link
Member Author

ematipico commented Jul 31, 2023

Have we considered what happens when the port is busy with this change? Currently, if I run astro dev, but another project is already running, Astro automatically increments the port number and uses 3001 instead (or 3002 etc. until it finds a free port). Is this now 4321, 4322, 4323?

That logic hasn't changed. So we incrementally find free ports like you said: 4321, 4322, etc.

@ematipico ematipico requested a review from delucis July 31, 2023 12:42
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ema, looks good. We could mention our motivation in relation to @fflaten's comment, e.g. adding "in order to avoid conflicts with ports used by other tools" or something to the changelog.

I've never really cared one way or the other about this change, but docs look ok to me!

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's still more places to change when I global-search for 3000, e.g.

  • sandbox.config.json ("port": 3000)
  • astro.config.mjs (site: 'http://localhost:3000')
  • Some READMEs

Since CI doesn't run all the integration tests, it would be good to confirm if they pass too locally.

packages/astro/src/@types/astro.ts Show resolved Hide resolved
@github-actions github-actions bot added pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) labels Jul 31, 2023
@ematipico ematipico force-pushed the feat/change-default-port-plt-706 branch from 4d93acd to 5d23182 Compare July 31, 2023 15:05
@fflaten
Copy link
Contributor

fflaten commented Jul 31, 2023

All README files says 4000 atm

@ematipico ematipico force-pushed the feat/change-default-port-plt-706 branch from 5d23182 to e71027f Compare July 31, 2023 16:01
@ematipico
Copy link
Member Author

All README files says 4000 atm

Cheers for the review. It should be OK now.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's more instances of 3000 in devcontainer.json:

  "portsAttributes": {
    "3000": {
      "label": "Application",
      "onAutoForward": "openPreview"
    }
  },

  "forwardPorts": [3000],

.gitpod.yml

ports:
  - port: 3000

And maybe the prefetch integration playwright.config.mjs:

		baseURL: process.env.PLAYWRIGHT_TEST_BASE_URL || 'http://localhost:3000',

@ematipico ematipico force-pushed the feat/change-default-port-plt-706 branch from 1f960e9 to b14a807 Compare August 1, 2023 15:03
@ematipico ematipico requested a review from bluwy August 1, 2023 15:24
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@ematipico ematipico merged commit 1b7dd11 into next Aug 1, 2023
17 checks passed
@ematipico ematipico deleted the feat/change-default-port-plt-706 branch August 1, 2023 15:31
ematipico added a commit that referenced this pull request Aug 3, 2023
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
ematipico added a commit that referenced this pull request Aug 8, 2023
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants