-
-
Notifications
You must be signed in to change notification settings - Fork 244
fix: replace hardcoded npmx.dev & docs.npmx.dev with env-specific URLs #1402
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { NPMX_DOCS_SITE_PROD } from '#shared/utils/constants' | ||
|
|
||
| export function useAppUrls() { | ||
| const { env, siteUrl } = useAppConfig() | ||
| return { | ||
| siteUrl, | ||
| // TODO(serhalp): Handle preview environment. The docs site is a separate deployment, so we'd | ||
| // need to infer its preview URL from the site preview URL somehow...? | ||
| docsUrl: env === 'dev' ? 'http://localhost:3001' : NPMX_DOCS_SITE_PROD, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import Git from 'simple-git' | ||
| import * as process from 'node:process' | ||
| import { NPMX_SITE_PROD } from '../shared/utils/constants' | ||
|
|
||
| export { version } from '../package.json' | ||
|
|
||
|
|
@@ -87,6 +88,11 @@ export const getProductionUrl = () => | |
| : undefined | ||
| : undefined | ||
|
|
||
| export const getSiteUrl = (isDevelopment: boolean) => { | ||
| if (isDevelopment) return 'http://localhost:3000' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wondering if we should or should not hardcode this? It is possible to pass flags with the dev command to change the port/host. Could we get that from somewhere else?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unfortunately only 127.0.0.1 works for the atproto oauth callback but yes, you can get the actual host and port in module space
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as I have a feature PR stacked on this one and PRs get stale fast here 😅 and this PR leaves things no worse than before, how would you feel about following up on this later? |
||
| return getPreviewUrl() || getProductionUrl() || NPMX_SITE_PROD | ||
| } | ||
|
|
||
| const git = Git() | ||
| export async function getGitInfo() { | ||
| let branch | ||
|
|
@@ -140,12 +146,14 @@ export async function getEnv(isDevelopment: boolean) { | |
| : 'release' | ||
| const previewUrl = getPreviewUrl() | ||
| const productionUrl = getProductionUrl() | ||
| const siteUrl = getSiteUrl(isDevelopment) | ||
| return { | ||
| commit, | ||
| shortCommit, | ||
| branch, | ||
| env, | ||
| previewUrl, | ||
| productionUrl, | ||
| siteUrl, | ||
| } as const | ||
| } | ||
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.
https://vercel.com/docs/deployments/generated-urls#generated-from-git
the URL's are deterministic, so can we just construct them during the build?
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.
as I have a feature PR stacked on this one and PRs get stale fast here 😅 and this PR leaves things no worse than before, how would you feel about following up on this later?