✨ feat(font): add assetsPrefix support for CDN deployments#30
Conversation
WalkthroughThe pull request updates the font configuration in the Astro component and its utility functions. In Changes
Sequence Diagram(s)sequenceDiagram
participant AF as AstroFont Component
participant AP as Astro Props
participant CM as Config Modifier
participant GF as generateFonts
AF->>AP: Retrieve originalConfig and assetsPrefix
AP-->>AF: Return props values
AF->>CM: Map originalConfig to configWithPrefix (adds assetsPrefix)
CM-->>AF: Return updated configurations
AF->>GF: Call generateFonts(configWithPrefix)
GF-->>AF: Process font generation
sequenceDiagram
participant Caller as Preload/CSS Generator
participant GPU as generatePublicUrl
Caller->>GPU: Call with localPath, basePath, and [assetsPrefix]
GPU->>GPU: Check if localPath is absolute?
alt localPath is absolute
GPU-->>Caller: Return absolute localPath
else localPath is relative
GPU->>GPU: Compute relative path and adjust assetsPrefix (ensure trailing slash, etc.)
GPU-->>Caller: Return constructed public URL
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/astro-font/utils.ts (1)
1-428: Consider adding a unit test for the new URL generation logic.The implementation looks solid, but it would be beneficial to add tests specifically for the
generatePublicUrlfunction to verify it handles different edge cases correctly:
- URLs with and without the prefix
- Various combinations of slashes
- Absolute URLs
🧰 Tools
🪛 Biome (1.9.4)
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/astro-font/AstroFont.astro(1 hunks)packages/astro-font/utils.ts(3 hunks)
🔇 Additional comments (8)
packages/astro-font/AstroFont.astro (3)
5-8: Nice addition of theassetsPrefixproperty!The new
Propsinterface clearly defines the component's contract, making the optionalassetsPrefixparameter explicit for users.
10-16: Good approach for maintaining backward compatibility.The implementation properly handles the new
assetsPrefixproperty by:
- Destructuring the props with appropriate renaming
- Creating a new configuration that includes the prefix in each font config
This ensures the prefix is properly propagated to all utility functions.
18-18: LGTM! Proper usage of the enhanced config.The
generateFontsfunction is now receiving the enhanced configuration with the assets prefix data.packages/astro-font/utils.ts (5)
60-60: LGTM! Interface updated correctly.The
Configinterface has been properly extended with the optionalassetsPrefixproperty.
75-77: Good refactoring of thegetBasePathfunction.The function is now more concise and readable.
79-95: Well-implemented URL generation function.The new
generatePublicUrlfunction:
- Correctly handles absolute URLs
- Properly normalizes both the prefix and the relative path
- Falls back to the original behavior when no prefix is provided
- Is well-commented with clear explanations
This is the core of the feature and has been implemented thoughtfully.
351-361: Good implementation of the preload URL generation.The function now correctly:
- Uses the new URL generation logic
- Extracts the base path calculation for clarity
- Passes the optional assets prefix
This ensures preload links will work correctly with CDN deployments.
363-375: CSS generation properly updated to use the new URL format.The
createBaseCSSfunction now correctly generates URLs with the assets prefix applied, ensuring that@font-facerules will reference fonts at the correct locations when using CDNs.
|
@rishi-raj-jain This would be a really helpful addition to this library! |
There was a problem hiding this comment.
Pull request overview
This PR adds support for CDN deployments by introducing an optional assetsPrefix configuration property to astro-font. This enables users to specify custom URL prefixes for font assets, allowing fonts to be served from CDNs or non-root paths as configured through Astro's build.assetsPrefix.
Changes:
- Added
assetsPrefixoptional property to theConfiginterface - Introduced
generatePublicUrlutility function to handle public URL generation with optional prefix support - Updated
createPreloadsandcreateBaseCSSto use the new URL generation logic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/astro-font/utils.ts | Added assetsPrefix to Config interface, implemented generatePublicUrl function for URL generation, and updated createPreloads and createBaseCSS to use the new URL generation logic |
| packages/astro-font/AstroFont.astro | Added assetsPrefix prop to component interface, implemented logic to propagate the prefix to all font configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface Config { | ||
| name: string | ||
| src: Source[] | ||
| fetch?: boolean | ||
| verbose?: boolean | ||
| selector?: string | ||
| preload?: boolean | ||
| cacheDir?: string | ||
| basePath?: string | ||
| fallbackName?: string | ||
| googleFontsURL?: string | ||
| cssVariable?: string | boolean | ||
| fallback: 'serif' | 'sans-serif' | 'monospace' | ||
| // https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-display | ||
| display: 'auto' | 'block' | 'swap' | 'fallback' | 'optional' | (string & {}) | ||
| assetsPrefix?: string | ||
| } |
There was a problem hiding this comment.
The Config interface needs to be exported to allow the import statement in AstroFont.astro (line 2) to work properly. The file currently imports Config from ./dist/utils, but since this interface is not exported, TypeScript compilation will fail or the import will not resolve correctly. Change interface Config to export interface Config to fix this issue.
| if (assetsPrefix) { | ||
| // Ensure prefix ends with a slash and relative path doesn't start with one | ||
| const normalizedPrefix = assetsPrefix.endsWith('/') ? assetsPrefix : assetsPrefix + '/'; | ||
| const normalizedRelative = pathRelativeToPublic.startsWith('/') ? pathRelativeToPublic.substring(1) : pathRelativeToPublic; | ||
| return normalizedPrefix + normalizedRelative; |
There was a problem hiding this comment.
The generatePublicUrl function should validate the assetsPrefix parameter to prevent potential security issues. If a user provides a malicious prefix containing special characters or path traversal sequences (e.g., javascript:, data:, or ../), this could lead to XSS vulnerabilities or unintended behavior. Consider adding validation to ensure the assetsPrefix starts with http://, https://, or /, and doesn't contain dangerous protocols or path traversal patterns.
| // Add the assetsPrefix passed to the component to each font config | ||
| assetsPrefix: assetsPrefix, |
There was a problem hiding this comment.
When assetsPrefix is undefined, the spread operator on line 15 will add assetsPrefix: undefined to each config object. While this works functionally, it's more explicit and cleaner to conditionally add the property only when it's defined. Consider using: ...(assetsPrefix ? { assetsPrefix } : {}) instead of assetsPrefix: assetsPrefix to avoid adding undefined properties to the config objects.
| // Add the assetsPrefix passed to the component to each font config | |
| assetsPrefix: assetsPrefix, | |
| // Add the assetsPrefix passed to the component to each font config, only when defined | |
| ...(assetsPrefix ? { assetsPrefix } : {}), |
| interface Props { | ||
| config: Config[]; | ||
| assetsPrefix?: string; | ||
| } |
There was a problem hiding this comment.
The Props interface is redefined locally in this file instead of extending the existing Props interface from utils.ts. This creates a discrepancy where the utils.ts exports Props with only a config property, but this file defines a different Props interface with both config and assetsPrefix. This could cause confusion and type inconsistencies. Consider either updating the exported Props interface in utils.ts to include the optional assetsPrefix property, or using a different name for this local interface (e.g., AstroFontProps) to avoid shadowing the exported type.
|
|
||
| if (assetsPrefix) { | ||
| // Ensure prefix ends with a slash and relative path doesn't start with one | ||
| const normalizedPrefix = assetsPrefix.endsWith('/') ? assetsPrefix : assetsPrefix + '/'; | ||
| const normalizedRelative = pathRelativeToPublic.startsWith('/') ? pathRelativeToPublic.substring(1) : pathRelativeToPublic; | ||
| return normalizedPrefix + normalizedRelative; | ||
| } else { | ||
| // Original behavior: return root-relative path, ensuring it starts with '/' | ||
| return '/' + (pathRelativeToPublic.startsWith('/') ? pathRelativeToPublic.substring(1) : pathRelativeToPublic); |
There was a problem hiding this comment.
The path normalization logic could produce incorrect results on Windows when pathRelativeToPublic contains backslashes. The relative() function from pathe should handle cross-platform paths, but the string manipulation with startsWith('/') and substring(1) assumes forward slashes. Consider normalizing the path separators before the slash checks, or use pathe's normalize() function to ensure consistent forward slashes across platforms. For example: const normalizedRelative = pathRelativeToPublic.replace(/\\/g, '/').replace(/^\//, '');
| if (assetsPrefix) { | |
| // Ensure prefix ends with a slash and relative path doesn't start with one | |
| const normalizedPrefix = assetsPrefix.endsWith('/') ? assetsPrefix : assetsPrefix + '/'; | |
| const normalizedRelative = pathRelativeToPublic.startsWith('/') ? pathRelativeToPublic.substring(1) : pathRelativeToPublic; | |
| return normalizedPrefix + normalizedRelative; | |
| } else { | |
| // Original behavior: return root-relative path, ensuring it starts with '/' | |
| return '/' + (pathRelativeToPublic.startsWith('/') ? pathRelativeToPublic.substring(1) : pathRelativeToPublic); | |
| // Normalize path separators and remove any leading slash for consistent URL construction | |
| const normalizedRelative = pathRelativeToPublic.replace(/\\/g, '/').replace(/^\//, ''); | |
| if (assetsPrefix) { | |
| // Ensure prefix ends with a slash and relative path doesn't start with one | |
| const normalizedPrefix = assetsPrefix.endsWith('/') ? assetsPrefix : assetsPrefix + '/'; | |
| return normalizedPrefix + normalizedRelative; | |
| } else { | |
| // Original behavior: return root-relative path, ensuring it starts with '/' | |
| return '/' + normalizedRelative; |
| if (localPath.startsWith('https:') || localPath.startsWith('http:')) { | ||
| return localPath; | ||
| } |
There was a problem hiding this comment.
The absolute URL detection only checks for https: and http: protocols, but doesn't handle protocol-relative URLs (starting with //) or other schemes like file://. While these cases may be rare in practice, consider using a more comprehensive check such as testing if the path contains :// or starts with // to handle all absolute URL formats consistently. For example: if (localPath.includes('://') || localPath.startsWith('//')) would be more robust.
Description:
This pull request introduces a new optional
assetsPrefixconfiguration property toastro-font. This feature allows users to specify a custom URL prefix for generated font assets (@font-facesrc URLs and<link rel="preload">hrefs), enabling seamless integration with CDNs or projects where assets are served from a non-root path, as configured via Astro'sbuild.assetsPrefix.Problem:
Currently,
astro-fontgenerates root-relative URLs for font assets (e.g.,/fonts/font.woff2). This works well for standard setups but breaks when using Astro'sbuild.assetsPrefixfeature to serve assets from a different domain (CDN) or a subpath (e.g.,https://mycdn.com/assets/or/my-base-path/). In such cases, the browser attempts to load fonts from the main site's root instead of the designated asset prefix, leading to 404 errors.While
astro-fontcorrectly uses local font paths for its internal analysis (calculating fallback metrics for CLS optimization), the final output URLs do not respect theassetsPrefix.Solution:
This PR implements the following changes:
assetsPrefix?: string: A new optional property is added to the mainConfiginterface withinutils.ts.assetsPrefixProp: The<AstroFont>component now accepts an optionalassetsPrefixprop. This single prop value is then merged into each individual font configuration object internally.generatePublicUrlUtility: A new functiongeneratePublicUrl(localPath, basePath, assetsPrefix)is created inutils.ts. This function handles the logic for generating the final public URL:assetsPrefixis provided, it prepends the prefix to the font path relative to the project's public directory (determined bybasePath).assetsPrefixis not provided, it defaults to the original behavior, generating a root-relative path (/path/to/font.woff2).https://...) passed insrc.path, returning them directly.createPreloadsandcreateBaseCSSfunctions now utilizegeneratePublicUrlto ensure the correct prefix is applied when generating<link>tags andsrc: url(...)values in@font-facerules.getFontBuffer,getFallbackFont) continues to use the local filesystem path provided insrc.path, ensuring the CLS optimization via fallback metric calculation remains unaffected and functions correctly even whenassetsPrefixis used. This also ensures compatibility with thefetch: trueoption (where the font is downloaded locally first, and theassetsPrefixis then applied to the local generated path).How to Use:
build.assetsPrefixor your CDN URL).<AstroFont>component:The resulting preload links and CSS URLs will now be correctly prefixed, e.g.:
href="https://mycdn.com/assets/fonts/Inter-Regular.woff2"src: url(https://mycdn.com/assets/fonts/Inter-Regular.woff2)Summary by CodeRabbit