-
Notifications
You must be signed in to change notification settings - Fork 0
fix: use npm Registry API instead of npm search #21
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
Changes from all commits
e3ead60
fb73774
ee379b0
e356bc4
20c70fe
33b050d
c30f1af
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 |
|---|---|---|
|
|
@@ -12,7 +12,11 @@ import tarFs from 'tar-fs'; | |
|
|
||
| import type { DiscoveredTemplate, FetchedTemplate, TemplatePackageJson } from './types.js'; | ||
|
|
||
| const TEMPLATE_SCOPE = '@nesalia/template-'; | ||
| // TEMPLATE_SCOPE is used for npm search API (without trailing dash) | ||
| const TEMPLATE_SCOPE = '@nesalia/template'; | ||
|
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. The TEMPLATE_SCOPE here is used correctly for filtering packages in the API response (line 67), but this matches the web app implementation. |
||
| // TEMPLATE_SCOPE_PACKAGE is used for constructing package names (with trailing dash) | ||
| const TEMPLATE_SCOPE_PACKAGE = '@nesalia/template-'; | ||
| const NPM_REGISTRY_API = `https://registry.npmjs.org/-/v1/search?text=${TEMPLATE_SCOPE}&size=100`; | ||
| const NPM_TIMEOUT = 60000; // 1 minute timeout for npm commands | ||
|
|
||
| // Use npm.cmd on Windows to handle batch files | ||
|
|
@@ -44,19 +48,25 @@ const execOptions = { | |
| */ | ||
| export const listTemplates = async (): Promise<DiscoveredTemplate[]> => { | ||
| try { | ||
| // Query npm registry for @nesalia/template-* packages | ||
| const result = execFileSync( | ||
| npmCmd, | ||
| ['search', '@nesalia/template', '--json', '--searchlimit=100'], | ||
| { ...execOptions, maxBuffer: 10 * 1024 * 1024 } | ||
| ); | ||
| // Use npm Registry API directly for faster and more reliable results | ||
| const response = await fetch(NPM_REGISTRY_API, { | ||
|
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. The timeout is set to 60000ms (1 minute), which is appropriate. However, |
||
| signal: AbortSignal.timeout(NPM_TIMEOUT), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`npm registry returned ${response.status}`); | ||
| } | ||
|
|
||
| const data = (await response.json()) as { | ||
| objects?: Array<{ name: string; description?: string; version: string }>; | ||
| }; | ||
|
|
||
| const packages = JSON.parse(result); | ||
| const packages = data.objects || []; | ||
|
|
||
| const templates: DiscoveredTemplate[] = packages | ||
| .filter((pkg: { name: string }) => pkg.name.startsWith(TEMPLATE_SCOPE)) | ||
| .filter((pkg: { name: string }) => pkg.name.startsWith(`${TEMPLATE_SCOPE}-`)) | ||
|
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 filter correctly checks for |
||
| .map((pkg: { name: string; description?: string; version: string }) => { | ||
| const id = pkg.name.replace(TEMPLATE_SCOPE, ''); | ||
| const id = pkg.name.replace(`${TEMPLATE_SCOPE}-`, ''); | ||
|
|
||
| // Parse description or use default | ||
| let description = pkg.description || ''; | ||
|
|
@@ -131,7 +141,7 @@ const validateTemplate = async ( | |
| } | ||
|
|
||
| // Validate nesalia.id matches expected format | ||
| const expectedId = packageName.replace(TEMPLATE_SCOPE, ''); | ||
| const expectedId = packageName.replace(TEMPLATE_SCOPE_PACKAGE, ''); | ||
| if (packageJson.nesalia.id !== expectedId) { | ||
| throw new Error( | ||
| `Invalid template: nesalia.id "${packageJson.nesalia.id}" does not match expected "${expectedId}"` | ||
|
|
@@ -225,7 +235,7 @@ export const fetchTemplate = async (templateId: string): Promise<FetchedTemplate | |
|
|
||
| const packageName = templateId.startsWith(TEMPLATE_SCOPE) | ||
| ? templateId | ||
| : `${TEMPLATE_SCOPE}${templateId}`; | ||
| : `${TEMPLATE_SCOPE_PACKAGE}${templateId}`; | ||
|
|
||
| // Validate package name format | ||
| if (!/^@[a-zA-Z0-9_-]+\/[a-zA-Z0-9_-]+$/.test(packageName)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,72 +3,252 @@ | |
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { listTemplates, fetchTemplate, cleanupTemplate } from '../src/registry.js'; | ||
| import { listTemplates } from '../src/registry.js'; | ||
|
|
||
| // Mock execFileSync | ||
| vi.mock('node:child_process', async () => { | ||
| const actual = await vi.importActual('node:child_process'); | ||
| return { | ||
| ...actual, | ||
| execFileSync: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| const mockExecFileSync = execFileSync as ReturnType<typeof vi.fn>; | ||
| // Mock fetch | ||
| const mockFetch = vi.fn(); | ||
| global.fetch = mockFetch; | ||
|
|
||
| describe('listTemplates', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should return templates from npm registry', async () => { | ||
| const mockNpmSearchResult = [ | ||
| { | ||
| name: '@nesalia/template-cli-py', | ||
| description: 'CLI Python - Python CLI with typer', | ||
| version: '1.0.0', | ||
| }, | ||
| { name: '@nesalia/template-react', description: 'React - React with Vite', version: '1.0.0' }, | ||
| { name: '@nesalia/template-vue', description: 'Vue - Vue 3 with Vite', version: '1.0.0' }, | ||
| ]; | ||
| it('should return templates from npm registry API', async () => { | ||
| const mockApiResponse = { | ||
| objects: [ | ||
| { | ||
| name: '@nesalia/template-cli-py', | ||
| description: 'CLI Python - Python CLI with typer', | ||
| version: '1.0.0', | ||
| }, | ||
| { | ||
| name: '@nesalia/template-react', | ||
| description: 'React - React with Vite', | ||
| version: '1.0.0', | ||
| }, | ||
| { | ||
| name: '@nesalia/template-vue', | ||
| description: 'Vue - Vue 3 with Vite', | ||
| version: '1.0.0', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| mockExecFileSync.mockReturnValue(JSON.stringify(mockNpmSearchResult)); | ||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue(mockApiResponse), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(mockExecFileSync).toHaveBeenCalledWith( | ||
| expect.stringMatching(/^npm(\.cmd)?$/), | ||
| expect.arrayContaining(['search', expect.stringContaining('@nesalia/template')]), | ||
| expect.any(Object) | ||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| 'https://registry.npmjs.org/-/v1/search?text=@nesalia/template&size=100', | ||
| expect.objectContaining({ signal: expect.any(AbortSignal) }) | ||
| ); | ||
| expect(templates).toHaveLength(3); | ||
| expect(templates[0].id).toBe('cli-py'); | ||
| expect(templates[0].displayName).toBe('CLI Python'); | ||
| expect(templates[0].version).toBe('1.0.0'); | ||
| }); | ||
|
|
||
| it('should throw error when npm query fails', async () => { | ||
| mockExecFileSync.mockImplementation(() => { | ||
| throw new Error('Network error'); | ||
| it('should return empty array when no templates found', async () => { | ||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue({ objects: [] }), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(templates).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('should throw error when API returns non-OK status', async () => { | ||
|
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. Good test coverage. However, there's a minor issue: the mock for |
||
| mockFetch.mockResolvedValue({ | ||
| ok: false, | ||
| status: 500, | ||
| }); | ||
|
|
||
| await expect(listTemplates()).rejects.toThrow('Failed to fetch templates from npm'); | ||
| }); | ||
|
|
||
| it('should throw error when fetch fails', async () => { | ||
| mockFetch.mockRejectedValue(new Error('Network error')); | ||
|
|
||
| await expect(listTemplates()).rejects.toThrow('Failed to fetch templates from npm'); | ||
| }); | ||
|
|
||
| it('should filter out non-nesalia packages', async () => { | ||
| const mockApiResponse = { | ||
| objects: [ | ||
| { | ||
| name: '@nesalia/template-cli-py', | ||
| description: 'CLI Python - Python CLI with typer', | ||
| version: '1.0.0', | ||
| }, | ||
| { | ||
| name: '@other/package', | ||
| description: 'Some other package', | ||
| version: '1.0.0', | ||
| }, | ||
| { | ||
| name: '@nesalia/template-monorepo', | ||
| description: 'Monorepo template', | ||
| version: '1.0.0', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue(mockApiResponse), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(templates).toHaveLength(2); | ||
| expect(templates.map(t => t.id)).toEqual(['cli-py', 'monorepo']); | ||
| }); | ||
|
|
||
| it('should parse displayName from description correctly', async () => { | ||
| const mockApiResponse = { | ||
| objects: [ | ||
| { | ||
| name: '@nesalia/template-electron-vite-react', | ||
| description: 'Electron + Vite + React - Desktop app template', | ||
| version: '1.0.0', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue(mockApiResponse), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(templates[0].displayName).toBe('Electron + Vite + React'); | ||
| expect(templates[0].description).toBe('Desktop app template'); | ||
| }); | ||
|
|
||
| it('should handle missing description gracefully', async () => { | ||
| const mockApiResponse = { | ||
| objects: [ | ||
| { | ||
| name: '@nesalia/template-test', | ||
| version: '1.0.0', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue(mockApiResponse), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(templates[0].displayName).toBe('test'); | ||
| expect(templates[0].description).toBe(''); | ||
| }); | ||
|
|
||
| it('should handle template starting with @nesalia/template prefix', async () => { | ||
| const mockApiResponse = { | ||
| objects: [ | ||
| { | ||
| name: '@nesalia/template-monorepo-ts', | ||
| description: 'TypeScript monorepo', | ||
| version: '1.0.0', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue(mockApiResponse), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(templates[0].id).toBe('monorepo-ts'); | ||
| }); | ||
|
|
||
| it('should include empty tags array', async () => { | ||
| const mockApiResponse = { | ||
| objects: [ | ||
| { | ||
| name: '@nesalia/template-test', | ||
| description: 'Test template', | ||
| version: '1.0.0', | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue(mockApiResponse), | ||
| }); | ||
|
|
||
| const templates = await listTemplates(); | ||
|
|
||
| expect(templates[0].tags).toEqual([]); | ||
| }); | ||
|
|
||
| it('should handle malformed JSON response', async () => { | ||
|
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. The test for malformed JSON response checks |
||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: vi.fn().mockRejectedValue(new Error('Parse error')), | ||
| }); | ||
|
|
||
| await expect(listTemplates()).rejects.toThrow('Failed to fetch templates from npm'); | ||
| }); | ||
| }); | ||
|
|
||
| // Tests for fetchTemplate validation (imported for testing) | ||
| import { fetchTemplate, cleanupTemplate } from '../src/registry.js'; | ||
| import { execFileSync } from 'node:child_process'; | ||
|
|
||
| const mockExecFileSync = execFileSync as ReturnType<typeof vi.fn>; | ||
|
|
||
| vi.mock('node:child_process', async () => { | ||
| const actual = await vi.importActual('node:child_process'); | ||
| return { | ||
| ...actual, | ||
| execFileSync: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| describe('fetchTemplate', () => { | ||
| it('should throw error for non-existent package', async () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should throw error for invalid template ID with spaces', async () => { | ||
| await expect(fetchTemplate('invalid id')).rejects.toThrow('Invalid template ID'); | ||
| }); | ||
|
|
||
| it('should throw error for path traversal attempt', async () => { | ||
| await expect(fetchTemplate('../etc/passwd')).rejects.toThrow('Invalid template ID'); | ||
| }); | ||
|
|
||
| it('should throw error for empty template ID', async () => { | ||
| await expect(fetchTemplate('')).rejects.toThrow('Invalid template ID'); | ||
| }); | ||
|
|
||
| it('should throw error when npm pack fails', async () => { | ||
| mockExecFileSync.mockImplementation(() => { | ||
| throw new Error('npm error: 404'); | ||
| }); | ||
|
|
||
| await expect(fetchTemplate('non-existent')).rejects.toThrow('npm error: 404'); | ||
| await expect(fetchTemplate('cli-py')).rejects.toThrow('npm error: 404'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('cleanupTemplate', () => { | ||
| it('should handle cleanup of non-existent directory', async () => { | ||
| // Should not throw | ||
| await cleanupTemplate('/non/existent/path'); | ||
| it('should handle cleanup errors gracefully', async () => { | ||
| // cleanupTemplate should not throw even if directory doesn't exist | ||
| // This is tested implicitly - if it threw, the test would fail | ||
| await expect(cleanupTemplate('/non/existent/path')).resolves.not.toThrow(); | ||
| }); | ||
| }); | ||
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.
Good use of constants for template scopes. However, there's a subtle inconsistency:
TEMPLATE_SCOPE(line 16) is@nesalia/templatebut the filtering logic (line 65) usesTEMPLATE_SCOPE-(with dash). Consider adding a comment or using a single constant to avoid confusion.