Conversation
📦 Package Size and Test Coverage Report
|
…ession for hostnames Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ession for hostnames Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
illiakovalenko
left a comment
There was a problem hiding this comment.
Please see my feedback and suggestions for improvement
| */ | ||
| const DEFAULT_EDGE_HOSTNAMES = [ | ||
| 'edge-platform.sitecorecloud.io', | ||
| 'edge.sitecorecloud.io', // Legacy hostname included for safety replacement |
There was a problem hiding this comment.
This is not a legacy URL - it is an Experience Edge URL.
We can leave default production url, but we cannot hardcode or enumerate custom environments here (staging, dev etc.). Customers and some internal teams may use additional environments, and with the current implementation they would not be able to configure custom hostnames.
To address this properly, we should introduce an internal environment variable: SITECORE_EXPERIENCE_EDGE_HOSTNAME. This variable would need to be populated by the Deploy team, allowing custom environment hostnames to be specified without constraining the implementation.
There was a problem hiding this comment.
@MenKNas This comment is not resolved, as no changes have been applied, except of moving to "core"
packages/create-content-sdk-app/src/templates/nextjs/sitecore.config.ts
Outdated
Show resolved
Hide resolved
packages/create-content-sdk-app/src/templates/nextjs/.env.remote.example
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm not sure this is applicable to local development with containers
| * Available on both server and client (e.g. NEXT_PUBLIC_* in Next.js). | ||
| * @public | ||
| */ | ||
| export const SITECORE_EDGE_HOSTNAME_PUBLIC_ENV = 'NEXT_PUBLIC_SITECORE_EDGE_HOSTNAME'; |
There was a problem hiding this comment.
Please refer to my previous comment where I pointed out that the core package should not reference Next.js–specific environment variables. This should instead be handled at the nextjs package level, for example via a defineConfig override. Also it's not renamed to "SITECORE_EDGE_PLATFORM_HOSTNAME"
| export function getDesignLibraryScriptLink(sitecoreEdgeUrl = SITECORE_EDGE_URL_DEFAULT): string { | ||
| return `${normalizeUrl(sitecoreEdgeUrl)}/v1/files/designlibrary/lib/rh-lib-script.js`; | ||
| export function getDesignLibraryScriptLink(sitecoreEdgeUrl?: string): string { | ||
| return `${(sitecoreEdgeUrl ? normalizeUrl(sitecoreEdgeUrl) : resolveEdgeUrl())}/v1/files/designlibrary/lib/rh-lib-script.js`; |
There was a problem hiding this comment.
Please refer to my earlier comment. The current approach still introduces unnecessary complexity without a clear justification.
As mentioned before, the edge URL should be resolved and normalized in a single place - at the configuration level. There is no need to repeat this logic every time the edge URL is passed as a parameter.
This applies to ALL occurrences
| */ | ||
| const DEFAULT_EDGE_HOSTNAMES = [ | ||
| 'edge-platform.sitecorecloud.io', | ||
| 'edge.sitecorecloud.io', // Legacy hostname included for safety replacement |
There was a problem hiding this comment.
@MenKNas This comment is not resolved, as no changes have been applied, except of moving to "core"
packages/core/src/constants.ts
Outdated
| @@ -4,6 +4,23 @@ | |||
| */ | |||
| export const SITECORE_EDGE_URL_DEFAULT = 'https://edge-platform.sitecorecloud.io'; | |||
There was a problem hiding this comment.
| export const SITECORE_EDGE_URL_DEFAULT = 'https://edge-platform.sitecorecloud.io'; | |
| export const SITECORE_EDGE_PLATFORM_URL_DEFAULT = 'https://edge-platform.sitecorecloud.io'; |
|
|
||
| # Optional: custom Experience Edge hostname override (XM Cloud/Edge; hostname or full URL). | ||
| # Available on both server and client. | ||
| NEXT_PUBLIC_SITECORE_EDGE_HOSTNAME= |
There was a problem hiding this comment.
| NEXT_PUBLIC_SITECORE_EDGE_HOSTNAME= | |
| NEXT_PUBLIC_SITECORE_EDGE_PLATFORM_HOSTNAME= |
| # Will be used as a fallback if separate SITECORE_EDGE_CONTEXT_ID value is not provided | ||
| NEXT_PUBLIC_SITECORE_EDGE_CONTEXT_ID= | ||
|
|
||
| # Optional: custom Experience Edge hostname override (XM Cloud/Edge; hostname or full URL). |
There was a problem hiding this comment.
| # Optional: custom Experience Edge hostname override (XM Cloud/Edge; hostname or full URL). | |
| # Optional: custom Sitecore Edge Platform hostname (hostname or full URL). |
| NEXT_PUBLIC_SITECORE_EDGE_CONTEXT_ID= | ||
|
|
||
| # Optional: custom Experience Edge hostname override (XM Cloud/Edge; hostname or full URL). | ||
| # Available on both server and client. |
There was a problem hiding this comment.
This can be removed, as it is already expected behavior for Next.js developers when using NEXT_PUBLIC environment variables.
| # Available on both server and client. |
|
|
||
| # Optional: custom Experience Edge hostname override (XM Cloud/Edge; hostname or full URL). | ||
| # Available on both server and client. | ||
| NEXT_PUBLIC_SITECORE_EDGE_HOSTNAME= |
There was a problem hiding this comment.
Please, see my comments for "nextjs" template as a references
There was a problem hiding this comment.
Please, add unit tests
CHANGELOG.md
Outdated
| - Available in both Pages Router and App Router templates | ||
|
|
||
| * `[core]` `[content]` `[nextjs]` Support custom Edge hostnames via `NEXT_PUBLIC_SITECORE_EDGE_HOSTNAME` ([#359](https://github.com/Sitecore/content-sdk/pull/359)) | ||
| - Consolidated `rewriteContentUrls` and `contentRewrite` into `rewriteMediaUrls: boolean | ((value: string) => string)`. When `true`, uses default Edge host rewriter; when a function, transforms each string (SDK traverses layout). Migration: `rewriteContentUrls: true` → `rewriteMediaUrls: true`; custom rewriter → `rewriteMediaUrls: (value: string) => string` |
There was a problem hiding this comment.
Could we make sure to double check the AI-generated notes? It should be removed as it was renamed and consolidated during the PR review, initially we didn't have this feature.
We can simply mention the new property that was introduced
Co-authored-by: Illia Kovalenko <23364749+illiakovalenko@users.noreply.github.com>
| `${normalizeUrl(sitecoreEdgeUrl)}/v1/content/api/graphql/v1`; | ||
| export const getEdgeProxyContentUrl = ( | ||
| sitecoreEdgeUrl: string = constants.SITECORE_EXPERIENCE_EDGE_URL_DEFAULT | ||
| ) => `${getBaseEdgeUrl(sitecoreEdgeUrl)}/v1/content/api/graphql/v1`; |
There was a problem hiding this comment.
Let me clarify again the difference between Experience Edge and Edge Platform URLs, as there appears to be a misunderstanding.
Experience Edge (https://edge.sitecorecloud.io)
– Used for media URLs.
Edge Platform (https://edge-platform.sitecorecloud.io)
– Used by our services to resolve endpoints.
Based on the changes you introduced, Experience Edge is now being referenced when resolving service endpoints. This is not correct. We should continue using the default Edge Platform URL for service-related requests.
The Experience Edge URL should be referenced only when replacing media URLs.
This is the only place where the Experience Edge URL environment variable should be used, in order to allow overriding it when necessary.
|
|
||
| const contextId = serverContextId || clientContextId; | ||
| // Use default Edge URL for styles (ignore custom hostname) so stylesheets load from platform | ||
| const edgeUrlForStyles = resolveEdgeUrlForStaticFiles(); |
There was a problem hiding this comment.
Are stylesheets not supported with a custom hostname? Is this an existing limitation?
There was a problem hiding this comment.
When using a custom hostname and attempting to serve styles, it resulted in a 404 error, and the styles on the page appeared broken. So I kept it this way to make sure styles don't break in this case.
| NEXT_PUBLIC_SITECORE_EDGE_CONTEXT_ID= | ||
|
|
||
| # Optional: custom Sitecore Edge Platform hostname (hostname or full URL). | ||
| # Next.js: use NEXT_PUBLIC_SITECORE_EDGE_PLATFORM_HOSTNAME for client; SITECORE_EDGE_PLATFORM_HOSTNAME for server-only. |
There was a problem hiding this comment.
| # Next.js: use NEXT_PUBLIC_SITECORE_EDGE_PLATFORM_HOSTNAME for client; SITECORE_EDGE_PLATFORM_HOSTNAME for server-only. |
| NEXT_PUBLIC_SITECORE_EDGE_CONTEXT_ID= | ||
|
|
||
| # Optional: custom Sitecore Edge Platform hostname (hostname or full URL). | ||
| # Next.js: use NEXT_PUBLIC_SITECORE_EDGE_PLATFORM_HOSTNAME for client; SITECORE_EDGE_PLATFORM_HOSTNAME for server-only. |
There was a problem hiding this comment.
| # Next.js: use NEXT_PUBLIC_SITECORE_EDGE_PLATFORM_HOSTNAME for client; SITECORE_EDGE_PLATFORM_HOSTNAME for server-only. |
| const DEFAULT_EDGE_HOSTNAMES = [ | ||
| 'edge-platform.sitecorecloud.io', | ||
| 'edge.sitecorecloud.io', | ||
| 'edge-staging.sitecore-staging.cloud', | ||
| 'edge-platform-staging.sitecore-staging.cloud', | ||
| ] as const; |
There was a problem hiding this comment.
Please refer to my previous comment - the issue has not been resolved yet
Description / Motivation
Support custom Edge hostnames
This PR adds support for custom Sitecore Edge hostnames by resolving the Edge base URL from
SITECORE_EDGE_PLATFORM_HOSTNAME(and, for Next.js,NEXT_PUBLIC_SITECORE_EDGE_PLATFORM_HOSTNAME) and ensuring SDK requests and layout/editing data can use the custom hostname instead of the default Edge Platform host. It also adds opt-in media URL rewriting so that URLs in layout data (e.g. image fields, rich text) can be rewritten to the custom hostname when desired.How it works
define-config): explicit config value →SITECORE_EDGE_PLATFORM_HOSTNAME/ Next.js public equivalent → default Edge Platform URL. Helpers receive the already-resolved URL and only normalize it; they do not read env or resolve again.rewrite-edge-host.ts).rewriteMediaUrls: trueis set in Sitecore config, the client applies the default Edge host rewriter to layout data before returning it (rewriting media URLs and rich text to the custom hostname). WhenrewriteMediaUrlsis a function, that function is used to transform each string in the layout.resolveEdgeUrlForStaticFiles()always returns the default Edge Platform URL so that static assets (e.g. stylesheets under/v1/files/...) continue to load from the platform when the custom host does not serve them.rewriteMediaUrlsto a custom(value: string) => stringto implement their own URL transformation over layout strings.Key files
packages/core/src/tools/resolve-edge-url.ts– Edge base URL resolution (used at config time);resolveEdgeUrlForStaticFiles()for static assets.packages/content/src/layout/rewrite-edge-host.ts– Rewrites default Edge hostnames in layout/editing responses to the resolved custom hostname; used as the default whenrewriteMediaUrls: true.packages/content/src/config/define-config.ts– Resolves edge URL at config level; defaultrewriteMediaUrls: false.packages/content/src/client/sitecore-client.ts– Applies therewriteMediaUrlsrewriter when enabled.Testing Details
Tested manually in sample app using custom hostname provided in https://portal-staging.sitecore-staging.cloud/admin/custom-hostnames?organization=org_Jk9RJbQPa0m9Psul (test3-staging.envoy-test.cloud)
Types of changes