fetchのパラメータを指定できるようにする。 #213
Conversation
- ExecuteOptionsインターフェースを追加 - 各API実行メソッドにoptions引数を追加 - NarouNovelFetchのfetchメソッドにfetchOptionsを渡すテストを追加
- ranking-history.test.tsにfetchOptionsを渡すテストを追加 - ranking.test.tsにfetchOptionsを渡すテストを追加 - search-builder-r18.test.tsにfetchOptionsを渡すテストを追加 - search-builder.test.tsにfetchOptionsを渡すテストを追加 - user-search.test.tsにfetchOptionsを渡すテストを追加
There was a problem hiding this comment.
Pull request overview
This PR adds support for passing fetch options to the Narou API client, addressing the issue where Cloudflare Workers' fetch doesn't include a User-Agent by default. The implementation introduces an ExecuteOptions interface that allows users to specify custom fetch parameters (like headers) when making API requests.
Key changes:
- Added
ExecuteOptionsinterface with optionalfetchOptionsproperty for passing RequestInit parameters - Propagated the options parameter through all API execution methods (search, ranking, user search, and ranking history)
- Added comprehensive test coverage to verify that custom fetch options are properly passed to requests
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/narou.ts | Defines ExecuteOptions interface and adds options parameter to all abstract and concrete execute methods |
| src/narou-fetch.ts | Implements options forwarding by passing fetchOptions to the fetch call |
| src/narou-jsonp.ts | Accepts but ignores options parameter (JSONP doesn't support custom fetch options) |
| src/search-builder.ts | Adds options parameter to execute() method in base and novel search builder classes, includes minor whitespace change |
| src/search-builder-r18.ts | Adds options parameter to R18 search builder's execute() method |
| src/user-search.ts | Adds options parameter to user search builder's execute() method |
| src/ranking.ts | Adds options parameter to execute() and all executeWithFields() overloads |
| src/index.ts | Adds options parameter to rankingHistory() function with updated JSDoc |
| test/search-builder.test.ts | Tests that fetchOptions are properly passed for regular novel searches |
| test/search-builder-r18.test.ts | Tests that fetchOptions are properly passed for R18 novel searches |
| test/user-search.test.ts | Tests that fetchOptions are properly passed for user searches |
| test/ranking.test.ts | Tests that fetchOptions are properly passed for ranking requests |
| test/ranking-history.test.ts | Tests that fetchOptions are properly passed for ranking history requests, adds test infrastructure setup |
| test/narou-fetch.test.ts | Tests that fetchOptions are correctly forwarded to the underlying fetch call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function rankingHistory( | ||
| ncode: string, | ||
| options?: ExecuteOptions, | ||
| api: NarouNovel = narouNovelFetch | ||
| ): Promise<RankingHistoryResult[]> { |
There was a problem hiding this comment.
The parameter order places options before api, which is inconsistent with common JavaScript/TypeScript patterns where optional configuration parameters typically come last. Consider reordering parameters to (ncode: string, api?: NarouNovel, options?: ExecuteOptions) or making options the last parameter as (ncode: string, options?: ExecuteOptions, api?: NarouNovel). However, placing the optional override parameter api last is more conventional, so the recommended signature would be: (ncode: string, options?: ExecuteOptions, api?: NarouNovel).
There was a problem hiding this comment.
これはAPIクライアントをできるだけ隠蔽したいのでこのまま
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async executeWithFields( | ||
| options?: ExecuteOptions | ||
| ): Promise<RankingResult<DefaultSearchResultFields>[]>; |
There was a problem hiding this comment.
The first overload executeWithFields(options?: ExecuteOptions) may cause ambiguity or breaking changes.
When calling executeWithFields() with no arguments, it's unclear which overload should be selected. More importantly, existing code that calls executeWithFields() expecting default fields will continue to work, but the semantics have changed - now the first parameter could be interpreted as options rather than fields.
TypeScript's overload resolution picks the first matching signature, which could lead to confusion. Consider these alternatives:
- Remove the first overload and require callers to explicitly pass an empty array for default fields:
executeWithFields([], undefined, options) - Use a different name for the options-only variant, like
executeWithFieldsAndOptions(options) - Change all overloads to accept options as the last parameter consistently, which is already done, and remove the ambiguous first overload
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export async function rankingHistory( | ||
| ncode: string, | ||
| options?: ExecuteOptions, | ||
| api: NarouNovel = narouNovelFetch | ||
| ): Promise<RankingHistoryResult[]> { |
There was a problem hiding this comment.
The new rankingHistory signature adds options ahead of the api parameter, so a call like rankingHistory("n0001", narouNovelJsonp)—previously supported—now treats the API instance as ExecuteOptions and silently falls back to the default fetch implementation. This breaks existing callers that supply a custom NarouNovel (e.g., the JSONP version) and prevents those instances from being used without changing call sites.
Useful? React with 👍 / 👎.
| fields: TFields | TFields[] = [], | ||
| opt?: TOpt | ||
| opt?: TOpt, | ||
| options?: ExecuteOptions | ||
| ): Promise<RankingResult<SearchResultFields<TFields>>[]> { |
There was a problem hiding this comment.
Overloads advertise executeWithFields(options?: ExecuteOptions), but the implementation keeps options as the third parameter. Calling the method with only execution options (the intended new use case) binds that object to fields, producing an of=[object Object]-ncode query and never forwarding fetchOptions to the API call. Options-only calls therefore malfunction unless the caller now passes placeholder field arguments or reshapes the signature.
Useful? React with 👍 / 👎.
Cloudflare WorkersのfetchではUserAgentが付かないのでfetchのパラメータを指定できるようにしたい。
追加パラメータでfetchのオプションをいい感じに指定できるようにする。