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

Add PORT env var to be used #952

Merged
merged 4 commits into from
Aug 11, 2021
Merged

Add PORT env var to be used #952

merged 4 commits into from
Aug 11, 2021

Conversation

stramel
Copy link
Contributor

@stramel stramel commented Jul 30, 2021

Resolves #872

Changes

  • Adds groundwork for allowing environment variables to be used by the CLI
  • Keeps CLI Flags as priority over environment variables, based on this post (flag > envvar > config > default)
  • Allows PORT environment variable to be used (PORT=4200 yarn dev)

Testing

Manually tested this by running the build and starting the cli directly from the astro.cjs file

Docs

I haven't updated this yet because I'm unsure where this information should live.

NOTE: I was attempting to set this up so we could easily support additional environment variables in the future without much effort. I could probably instead add just a simple change instead of the current PR like so:

-     port: typeof flags.port === 'number' ? flags.port : undefined,
+    port: typeof flags.port === 'number' ? flags.port : parseInt(PORT || '') || undefined,

So just let me know if I over-complicated it and I'll swap it out

@changeset-bot
Copy link

changeset-bot bot commented Jul 30, 2021

🦋 Changeset detected

Latest commit: cf5122e

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

This PR includes changesets to release 1 package
Name Type
astro Patch

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

@vercel
Copy link

vercel bot commented Jul 30, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

astro-www – ./www

🔍 Inspect: https://vercel.com/pikapkg/astro-www/63bpCAXnwdV9A4KtPt7PGkpNPEiq
✅ Preview: https://astro-www-git-fork-stramel-ms-add-env-var-options-pikapkg.vercel.app

astro-docs – ./docs

🔍 Inspect: https://vercel.com/pikapkg/astro-docs/AjNBZpmWRbJSUSdeBjjtGMkmPsTd
✅ Preview: https://astro-docs-git-fork-stramel-ms-add-env-var-options-pikapkg.vercel.app

@natemoo-re
Copy link
Member

Thanks so much for the contribution @stramel! I'm on board with this approach. Would you mind rebasing and marking this as ready for review?

Copy link
Contributor

@jasikpark jasikpark left a comment

Choose a reason for hiding this comment

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

lgtm, though i don't 100% get the types of everything 😅

would https://mael.dev/typanion/ be helpful or is it better to not pull in a whole dep for one task 😅?

@stramel
Copy link
Contributor Author

stramel commented Aug 11, 2021

I could definitely rewrite it with typanion if you would like. I will leave it up to you all

@natemoo-re natemoo-re merged commit e104402 into withastro:main Aug 11, 2021
FredKSchott pushed a commit that referenced this pull request Aug 11, 2021
* Add PORT env var to be used

* Format

* Create few-carpets-sing.md

* Update few-carpets-sing.md
FredKSchott added a commit that referenced this pull request Aug 11, 2021
FredKSchott added a commit that referenced this pull request Aug 11, 2021
@stramel
Copy link
Contributor Author

stramel commented Aug 12, 2021

@FredKSchott did this break something? Saw it got reverted, just wondering if I need to fix something with this and re-issue a PR?

@FredKSchott
Copy link
Member

Ah sorry, I got pulled away right after reverting. Check this thread out: https://discord.com/channels/830184174198718474/845430950191038464/875149362928443402

I also missed that PR though, and have some thoughts :) I might recommend doing this more flexibly via config, since that then sets any user up to do port: process.env.PORT in their config file. The reason is that PORT is very vague, and could already be set due to any number of other projects on the user's machine.

I know this isn't ideal way to leave this feedback (and I should have gotten it to you sooner) Happy to chat in literally any other venue (discord thread or RFC) if you'd like to keep discussing.

SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* Add PORT env var to be used

* Format

* Create few-carpets-sing.md

* Update few-carpets-sing.md
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
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.

💡 RFC: Allow dev port to be set via environment variable
5 participants