-
-
Notifications
You must be signed in to change notification settings - Fork 246
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, | ||
| } | ||
| } | ||
|
Comment on lines
+3
to
+11
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. I'm not sure how useful this is - we can't get the preview url of a docs site, and we don't have a hard-coded port for the docs running locally (and this ends up being runtime code we don't really need in the site) |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,7 +129,8 @@ function handleClearFilter(chip: FilterChip) { | |
| const activeTab = shallowRef<'members' | 'teams'>('members') | ||
|
|
||
| // Canonical URL for this org page | ||
| const canonicalUrl = computed(() => `https://npmx.dev/@${orgName.value}`) | ||
| const { siteUrl } = useAppUrls() | ||
| const canonicalUrl = computed(() => `${siteUrl}/@${orgName.value}`) | ||
|
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. this ends up meaning that preview deployments get their own canonical url, and I would try to avoid that. just like with og images, you want to point to the canonical https://npmx.dev so that search engines don't end up indexing preview deployments accidentally (I know we have a header instructing them not to, but this seems safe to hard-code. if we ever need to switch https://npmx.dev, we could easily do it throughout the codebase without much effort.) |
||
|
|
||
| useHead({ | ||
| link: [{ rel: 'canonical', href: canonicalUrl }], | ||
|
|
||
| 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
Member
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?