-
Notifications
You must be signed in to change notification settings - Fork 0
Fix lint error in narou-jsonp #209
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: master
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.
Pull request overview
This PR adds support for passing fetch options (like custom headers or HTTP methods) to API calls by introducing an ExecuteOptions type parameter throughout the API surface. The main feature addition is accompanied by test infrastructure improvements (MSW mocking) and a lint fix for the JSONP implementation.
Key changes:
- Introduces
ExecuteOptionstype withfetchOptions?: RequestInitto enable custom fetch configuration - Fixes lint error in
narou-jsonp.tsby explicitly voiding the unusedoptionsparameter with interface compatibility comment - Migrates
narou.test.tsfrom external API calls to MSW mocking for more reliable and faster tests
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/narou.ts | Defines ExecuteOptions type and threads it through all abstract execute methods |
| src/narou-fetch.ts | Implements ExecuteOptions by passing fetchOptions to the fetch call |
| src/narou-jsonp.ts | Accepts ExecuteOptions for interface compatibility but voids it with explanatory comment |
| src/search-builder.ts | Updates execute() to accept and pass through ExecuteOptions |
| src/search-builder-r18.ts | Updates execute() to accept and pass through ExecuteOptions |
| src/user-search.ts | Updates execute() to accept and pass through ExecuteOptions |
| src/ranking.ts | Updates execute() and all executeWithFields() overloads to accept and pass through ExecuteOptions |
| src/index.common.ts | Exports ExecuteOptions type for public API consumption |
| src/index.ts | Changes error throwing from new Error(result) to raw result string |
| test/narou.test.ts | Migrates from external API calls to MSW mocking with server setup |
| test/narou-fetch.test.ts | Adds test coverage for fetchOptions parameter passing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MEMO: このファイルのテストは外部APIを利用するため、結果が変わる可能性がある。 | ||
| // そのため、結果が変わる可能性が少ないものを選択してテストを行っているが、落ちるようになったら修正が必要。 |
Copilot
AI
Dec 10, 2025
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 comment is now outdated. The tests no longer use external APIs since MSW (Mock Service Worker) is now being used to mock the API responses. Consider updating the comment to reflect that the tests now use mocked responses, or remove it entirely.
| // MEMO: このファイルのテストは外部APIを利用するため、結果が変わる可能性がある。 | |
| // そのため、結果が変わる可能性が少ないものを選択してテストを行っているが、落ちるようになったら修正が必要。 |
| return result.map(formatRankingHistory); | ||
| } else { | ||
| throw new Error(result); | ||
| throw result; |
Copilot
AI
Dec 10, 2025
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.
Throwing a raw string is not best practice. It's better to throw an Error object (throw new Error(result)) because Error objects provide stack traces and follow JavaScript/TypeScript conventions for error handling. The original code was correct in this regard.
| throw result; | |
| throw new Error(String(result)); |
| // まれに時間がかかるので30秒に設定 | ||
| vi.setConfig({ testTimeout: 30000 }); | ||
|
|
Copilot
AI
Dec 10, 2025
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.
[nitpick] This comment about test timeout is likely outdated now that MSW mocks are being used instead of real external API calls. Mocked responses should be fast and deterministic, so the 30-second timeout may no longer be necessary. Consider reducing it to a more typical value (e.g., 5-10 seconds) or using the default timeout.
| // まれに時間がかかるので30秒に設定 | |
| vi.setConfig({ testTimeout: 30000 }); |
Summary
Testing
Codex Task