-
-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add npm provider to support locally installed packages #189
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?
Conversation
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.
this looks amazing and I can't wait to get it merged!
a few comments.
src/providers/npm.ts
Outdated
async function detectFontsourcePackages(baseDir: string): Promise<string[]> { | ||
try { | ||
// Check if @fontsource directory exists | ||
const fontsourceDir = join(baseDir, '@fontsource') | ||
if (!existsSync(fontsourceDir)) { | ||
return [] | ||
} | ||
|
||
// Read the directory to find installed font packages | ||
const { readdir } = await import('node:fs/promises') | ||
const files = await readdir(fontsourceDir) | ||
|
||
return files.map(file => `@fontsource/${file}`) | ||
} | ||
catch (error) { | ||
// In case of any error, return empty array | ||
console.error('Failed to detect @fontsource packages:', error) | ||
return [] | ||
} | ||
} | ||
|
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.
let's instead try to check the package.json
of the workspace - we can use pkg-types
for this. And ideally I think we can also support other common fonts like cal-sans
, geist, etc.
src/providers/npm.ts
Outdated
const fontFamily | ||
= pkgJson.fontName | ||
|| pkgJson.fontFamily | ||
|| (pkgJson.name && pkgJson.name.replace(/^@fontsource\//, '')) |
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.
I'd like to remove the fontsource 'special-handling', if possible, and come up with a solution that works for fontsource but also conceivable supports auto-detection for other providers
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.
thank you for the feedback Daniel, I have just made the changes to support generic multiple font packages :)
I think this provider shouldn't be only called npm, but still mention fontsource eg. |
Ah sorry I missed this is supposed to work non fontsource packages too |
I'm wondering if it's really necessary for us to explicitly support third-party packages like Geist. Perhaps a more scalable approach would be to expose an API like Next.js does and let packages integrate with us instead. Support for Fontsource packages would be great though, given that they're already a well established and standardized system. |
Yeah that seems more scalable in long term, do you reckon we keep basic support for Fontsource and expose API for other packages? This hybrid approach can potentially bring extra complexity though, I'm not very experienced with this yet, I would let the reviewers make the reviewers make the decision. |
Resolves #188
Description
This PR adds a new npm provider that allows Unifont to resolve fonts from locally installed npm packages (especially @fontsource packages) before attempting to fetch them from remote CDNs. This improves performance, reduces network requests, and provides better offline support.
Key Changes
Testing
The implementation includes:
Usage Example
Additional Notes