fix: use npm Registry API instead of npm search#21
Conversation
Add coverage dependency for running tests with coverage reports. Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Replace execFileSync/npm search with direct fetch to npm Registry API for faster and more reliable template discovery. This fixes the issue where newly published templates don't appear in --list due to npm's delayed search indexing. The web app already uses this approach, now the CLI matches it. Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Add comprehensive tests for listTemplates function covering: - Successful template retrieval - Empty results handling - Error handling (non-OK status, network failures) - Package filtering (non-nesalia packages) - Description parsing - Edge cases Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
||
| 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.
Good use of constants for template scopes. However, there's a subtle inconsistency: TEMPLATE_SCOPE (line 16) is @nesalia/template but the filtering logic (line 65) uses TEMPLATE_SCOPE- (with dash). Consider adding a comment or using a single constant to avoid confusion.
|
|
||
| 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.
This filter correctly checks for TEMPLATE_SCOPE- (with dash), but it's worth noting this relies on the API returning packages with the @nesalia/template- prefix. Consider documenting this expectation or adding a comment explaining why the filter works this way.
| { ...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.
The timeout is set to 60000ms (1 minute), which is appropriate. However, AbortSignal.timeout() in Node.js may not work in older Node versions. Consider verifying the minimum Node.js version supported (currently specified as >=20 in package.json, which supports this feature).
| it('should throw error when npm query fails', async () => { | ||
| mockExecFileSync.mockImplementation(() => { | ||
| throw new Error('Network error'); | ||
| it('should throw error when API returns non-OK status', async () => { |
There was a problem hiding this comment.
Good test coverage. However, there's a minor issue: the mock for ok: false at line 68 should also mock statusText for completeness, as the actual Response object includes this property. Also consider adding a test for pagination if the API might return more than 100 results.
Update pnpm-lock.yaml to include new dependencies. Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Add explicit type annotation for the npm Registry API response to fix TypeScript error during build. Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
| expect(templates[0].tags).toEqual([]); | ||
| }); | ||
|
|
||
| it('should handle malformed JSON response', async () => { |
There was a problem hiding this comment.
The test for malformed JSON response checks response.ok is true but json() rejects. This is correct behavior, but worth noting the test covers JSON parse errors well.
SummaryThis PR replaces the Critical Issues
Recommendations
Positive Notes
Code Quality
Review by Claude Code |
- Add tests for fetchTemplate validation (invalid ID, path traversal, empty) - Add test for npm pack failure handling - Add cleanupTemplate error handling test - Add test:coverage script to package.json This addresses the test coverage regression noted in PR review. Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
|
|
||
| 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.
The TEMPLATE_SCOPE here is used correctly for filtering packages in the API response (line 67), but this matches the web app implementation.
SummaryThis PR replaces the CLI's use of Critical IssuesNone identified - the implementation is solid and addresses the stated issue of npm's delayed search indexing. Recommendations
Positive Notes
Test CoverageThe test suite is thorough:
Overall: This is a well-implemented fix that addresses the search indexing delay issue. The code is clean, secure, and well-tested. The minor recommendations are optional improvements - the PR is ready for merge. |
Summary
npm search(via execFileSync) with direct fetch to npm Registry API for template discovery--listdue to npm's delayed search indexingChanges
packages/create/src/registry.ts:
https://registry.npmjs.org/-/v1/searchpackages/create/package.json:
packages/create/tests/registry.test.ts:
Test plan
npm test- all tests passnpx @nesalia/create --listto verify templates appear🤖 Generated with Claude Code