Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds WOFF2 format support for font downloads alongside the existing TTF format, allowing users to download fonts optimized for web use. The feature is introduced via a new --woff2 CLI flag and includes backward-compatible API changes.
Key changes:
- Added
--woff2flag to the download command for selecting font format - Extended MIME type and file extension validation to support WOFF2 files
- Updated core library functions with backward-compatible format parameter support
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bumped from 2.0.2 to 2.1.0 to reflect new feature |
| package-lock.json | Lockfile updated to match new package version |
| lib/system-font.js | Extended file validation to accept WOFF2 MIME type and file extension |
| lib/google-font.js | Added format parameter to _getFileMap and saveAt methods with backward compatibility |
| cli.js | Added --ttf and --woff2 options to download command with format selection logic |
| README.md | Updated documentation with new format options and WOFF2 usage example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var isValidFont = false; | ||
| if (mimeType && (mimeType.mime === 'application/font-sfnt' || mimeType.mime === 'font/woff2')) { | ||
| isValidFont = true; | ||
| } else if (ext === '.ttf' || ext === '.woff2') { | ||
| isValidFont = true; | ||
| } | ||
|
|
||
| if (isValidFont) { |
There was a problem hiding this comment.
The logic structure could be simplified. Instead of using nested if-else statements and a separate isValidFont flag, consider consolidating the validation into a single condition. The current implementation is unnecessarily verbose and could be refactored for better readability.
| var isValidFont = false; | |
| if (mimeType && (mimeType.mime === 'application/font-sfnt' || mimeType.mime === 'font/woff2')) { | |
| isValidFont = true; | |
| } else if (ext === '.ttf' || ext === '.woff2') { | |
| isValidFont = true; | |
| } | |
| if (isValidFont) { | |
| var isValid = | |
| (mimeType && (mimeType.mime === 'application/font-sfnt' || mimeType.mime === 'font/woff2')) || | |
| (ext === '.ttf' || ext === '.woff2'); | |
| if (isValid) { |
|
|
||
| ``` | ||
| $ gfcli download [family_name] [-d|--dest destination_folder] [-v|--variants comma_separeted_variants] | ||
| $ gfcli download [family_name] [-d|--dest destination_folder] [-v|--variants comma_separeted_variants] [--ttf|--woff2] |
There was a problem hiding this comment.
There is a spelling error: 'comma_separeted_variants' should be 'comma_separated_variants'.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| json.variants.forEach(function(v) { | ||
| // Map variants to IDs | ||
| // prioritize TTF for system installation | ||
| if (v.ttf) { | ||
| // Map variants to IDs based on requested format | ||
| if (format === 'woff2' && v.woff2) { | ||
| files[v.id] = v.woff2; | ||
| } else if (format === 'ttf' && v.ttf) { | ||
| files[v.id] = v.ttf; | ||
| } |
There was a problem hiding this comment.
The format validation only handles 'woff2' and 'ttf' formats explicitly. If an invalid format is passed (e.g., 'otf', 'svg'), the code will still proceed but may produce an empty file list. Consider adding validation to reject unsupported formats with a clear error message, or at least document the expected format values.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| await ensureFontsLoaded(); | ||
| const term = family.join(" "); | ||
| const variants = options.variants ? options.variants.split(",") : false; | ||
| const format = options.woff2 ? "woff2" : "ttf"; |
There was a problem hiding this comment.
When both --ttf and --woff2 flags are provided simultaneously, the current logic will only respect --woff2 (due to the ternary operator checking woff2 first). This could be confusing to users. Consider either making these options mutually exclusive using Commander's .conflicts() method, or documenting that --woff2 takes precedence over --ttf.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Support both old (callback) and new (format, callback) signatures | ||
| if (typeof format === 'function') { | ||
| callback = format; | ||
| format = 'ttf'; // default format |
There was a problem hiding this comment.
Consider documenting the backward compatibility approach in the function's docstring or inline comments. While the signature handling is correct, it would be helpful for future maintainers to understand that this function supports both old (callback-only) and new (format, callback) signatures for backward compatibility.
| // Support both old (callback) and new (format, callback) signatures | |
| if (typeof format === 'function') { | |
| callback = format; | |
| format = 'ttf'; // default format | |
| // NOTE: This method is intentionally backward compatible: | |
| // - Legacy usage: _getFileMap(callback) | |
| // - New usage: _getFileMap(format, callback) | |
| // When called with a single function argument, that argument is treated as | |
| // the callback and the font format defaults to 'ttf'. The subsequent | |
| // `format = format || 'ttf';` also ensures that callers who explicitly pass | |
| // a falsy format (e.g. null/undefined) still get 'ttf' as the default. | |
| if (typeof format === 'function') { | |
| callback = format; | |
| format = 'ttf'; // default format when only callback is supplied |
Add woff2 support for download using --woff2 #2