Skip to content

Conversation

@deflis
Copy link
Owner

@deflis deflis commented Dec 10, 2025

Summary

  • mark JSONP execute options parameter as intentionally unused while retaining interface compatibility
  • ensure lint passes by voiding the unused ExecuteOptions argument

Testing

  • pnpm run check:lint

Codex Task

Copilot AI review requested due to automatic review settings December 10, 2025 04:15
Copy link
Contributor

Copilot AI left a 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 ExecuteOptions type with fetchOptions?: RequestInit to enable custom fetch configuration
  • Fixes lint error in narou-jsonp.ts by explicitly voiding the unused options parameter with interface compatibility comment
  • Migrates narou.test.ts from 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.

Comment on lines 88 to 89
// MEMO: このファイルのテストは外部APIを利用するため、結果が変わる可能性がある。
// そのため、結果が変わる可能性が少ないものを選択してテストを行っているが、落ちるようになったら修正が必要。
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
// MEMO: このファイルのテストは外部APIを利用するため、結果が変わる可能性がある。
// そのため、結果が変わる可能性が少ないものを選択してテストを行っているが、落ちるようになったら修正が必要。

Copilot uses AI. Check for mistakes.
return result.map(formatRankingHistory);
} else {
throw new Error(result);
throw result;
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
throw result;
throw new Error(String(result));

Copilot uses AI. Check for mistakes.
Comment on lines 91 to 93
// まれに時間がかかるので30秒に設定
vi.setConfig({ testTimeout: 30000 });

Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
// まれに時間がかかるので30秒に設定
vi.setConfig({ testTimeout: 30000 });

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants