fix: enable experimental.useTypeScriptCli for TypeScript 7 - #1576
Merged
Conversation
|
@Marukome0743 is attempting to deploy a commit to the OpenUp Lab Takizawa Team on Vercel. A member of the Team first needs to authorize it. |
🪄 Deploy Preview for ready!
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
⚙️config
Reviewer's GuideこのPRは、Next.jsにおけるTypeScript 7向けのTypeScript CLI経由の型チェックを有効化し、より厳格なCLIチェックによって顕在化した複数の単体テストの型問題を修正します。修正内容は主に、オプショナルな値、構造的キャスト、モックのシグネチャに関連するものです。 TypeScript CLI を用いた Next.js ビルドのシーケンス図sequenceDiagram
actor Developer
participant NextBuild
participant TypeScriptCli
Developer->>NextBuild: next build
NextBuild->>TypeScriptCli: run TypeScript type_check
TypeScriptCli-->>NextBuild: type_check result
NextBuild-->>Developer: build output
File-Level Changes
Tips and commandsSourcery とのやり取り
カスタマイズ方法ダッシュボード にアクセスして、次のことが行えます:
サポートを受けるには
Original review guide in EnglishReviewer's GuideThis PR enables Next.js TypeScript type-checking via the TypeScript CLI for TS 7 and fixes several unit-test type issues uncovered by the stricter CLI checks, mostly around optional values, structural casts, and mock signatures. Sequence diagram for Next.js build using the TypeScript CLIsequenceDiagram
actor Developer
participant NextBuild
participant TypeScriptCli
Developer->>NextBuild: next build
NextBuild->>TypeScriptCli: run TypeScript type_check
TypeScriptCli-->>NextBuild: type_check result
NextBuild-->>Developer: build output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 1つ問題を見つけたので、いくつか高レベルなフィードバックを残します:
- db のテストで繰り返し使われている
(db.constructor as unknown as Record<symbol, string>)[entityKind]というパターンは、小さなヘルパー関数か型エイリアスに切り出すことで、危険なキャストを一箇所に集約しつつ、可読性も向上させられます。 - S3 のテストで
s3.config.endpoint?.()がundefinedを返したときに、汎用的なエラーを投げる代わりに、expect(resolvedEndpoint).toBeDefined()のような明示的なアサーションを使うと、テストの意図がより明確になり、手動でエラーを構築する必要もなくなります。 - proxy のテストで
state.redirectPath = null as string | nullを使う代わりに、変数宣言時点でredirectPathをstring | null型として定義しておくと、代入時にアサーションキャストを使う必要がなくなり、よりクリーンになります。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- db のテストで繰り返し使われている `(db.constructor as unknown as Record<symbol, string>)[entityKind]` というパターンは、小さなヘルパー関数か型エイリアスに切り出すことで、危険なキャストを一箇所に集約しつつ、可読性も向上させられます。
- S3 のテストで `s3.config.endpoint?.()` が `undefined` を返したときに、汎用的なエラーを投げる代わりに、`expect(resolvedEndpoint).toBeDefined()` のような明示的なアサーションを使うと、テストの意図がより明確になり、手動でエラーを構築する必要もなくなります。
- proxy のテストで `state.redirectPath = null as string | null` を使う代わりに、変数宣言時点で `redirectPath` を `string | null` 型として定義しておくと、代入時にアサーションキャストを使う必要がなくなり、よりクリーンになります。
## Individual Comments
### Comment 1
<location path="test/unit/lib/storage/s3-backend.test.ts" line_range="125-133" />
<code_context>
expect(s3.config.forcePathStyle).toBe(true)
- const resolvedEndpoint = await s3.config.endpoint()
+ const resolvedEndpoint = await s3.config.endpoint?.()
+ if (!resolvedEndpoint) {
+ throw new Error("endpoint should be resolved")
+ }
const url = new URL(endpoint)
</code_context>
<issue_to_address>
**suggestion (testing):** `endpoint` が定義されているという期待を表現するために、throw ではなくテスト用のアサーションを使ってください
ここでの失敗条件はテストが期待している挙動の一部なので、手動で例外を投げるよりも、アサーションによって明示的に表現する方が分かりやすく、より慣用的です。例えば `expect(resolvedEndpoint).toBeDefined()` を行った上で、その後のプロパティチェックに対しては非 null アサーションや型ガードを使うことで、「endpoint は必ず resolve されているべき」という意図を保ちつつ、テスト本体のカスタムエラー処理を取り除けます。
```suggestion
expect(s3.config.forcePathStyle).toBe(true)
const resolvedEndpoint = await s3.config.endpoint?.()
expect(resolvedEndpoint).toBeDefined()
const url = new URL(endpoint)
expect(resolvedEndpoint!.hostname).toBe(url.hostname)
expect(resolvedEndpoint!.protocol).toBe(url.protocol)
```
</issue_to_address>もっと役に立てるようにしてください！各コメントに対して 👍 または 👎 をクリックしていただけると、そのフィードバックを次回以降のレビュー改善に活用します。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The repeated
(db.constructor as unknown as Record<symbol, string>)[entityKind]pattern in the db tests could be wrapped in a small helper or type alias to centralize the unsafe cast and improve readability. - Instead of throwing a generic error when
s3.config.endpoint?.()returnsundefinedin the S3 tests, consider using an explicitexpect(resolvedEndpoint).toBeDefined()assertion to better communicate test intent and avoid manual error construction. - Rather than using
state.redirectPath = null as string | nullin the proxy tests, it would be cleaner to defineredirectPathwith astring | nulltype at its declaration so you don't need assertion casts on assignment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `(db.constructor as unknown as Record<symbol, string>)[entityKind]` pattern in the db tests could be wrapped in a small helper or type alias to centralize the unsafe cast and improve readability.
- Instead of throwing a generic error when `s3.config.endpoint?.()` returns `undefined` in the S3 tests, consider using an explicit `expect(resolvedEndpoint).toBeDefined()` assertion to better communicate test intent and avoid manual error construction.
- Rather than using `state.redirectPath = null as string | null` in the proxy tests, it would be cleaner to define `redirectPath` with a `string | null` type at its declaration so you don't need assertion casts on assignment.
## Individual Comments
### Comment 1
<location path="test/unit/lib/storage/s3-backend.test.ts" line_range="125-133" />
<code_context>
expect(s3.config.forcePathStyle).toBe(true)
- const resolvedEndpoint = await s3.config.endpoint()
+ const resolvedEndpoint = await s3.config.endpoint?.()
+ if (!resolvedEndpoint) {
+ throw new Error("endpoint should be resolved")
+ }
const url = new URL(endpoint)
</code_context>
<issue_to_address>
**suggestion (testing):** Use test assertions instead of throwing to express the expectation that `endpoint` is defined
Since the failure condition here is part of the test’s expectation, it’s clearer and more idiomatic to assert on it rather than manually throwing. For example, `expect(resolvedEndpoint).toBeDefined()` followed by a non-null assertion or type guard for later property checks preserves the intent that the endpoint must be resolved and removes the custom error handling in the test body.
```suggestion
expect(s3.config.forcePathStyle).toBe(true)
const resolvedEndpoint = await s3.config.endpoint?.()
expect(resolvedEndpoint).toBeDefined()
const url = new URL(endpoint)
expect(resolvedEndpoint!.hostname).toBe(url.hostname)
expect(resolvedEndpoint!.protocol).toBe(url.protocol)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
125
to
133
| expect(s3.config.forcePathStyle).toBe(true) | ||
|
|
||
| const resolvedEndpoint = await s3.config.endpoint() | ||
| const resolvedEndpoint = await s3.config.endpoint?.() | ||
| if (!resolvedEndpoint) { | ||
| throw new Error("endpoint should be resolved") | ||
| } | ||
| const url = new URL(endpoint) | ||
| expect(resolvedEndpoint.hostname).toBe(url.hostname) | ||
| expect(resolvedEndpoint.protocol).toBe(url.protocol) |
There was a problem hiding this comment.
suggestion (testing): endpoint が定義されているという期待を表現するために、throw ではなくテスト用のアサーションを使ってください
ここでの失敗条件はテストが期待している挙動の一部なので、手動で例外を投げるよりも、アサーションによって明示的に表現する方が分かりやすく、より慣用的です。例えば expect(resolvedEndpoint).toBeDefined() を行った上で、その後のプロパティチェックに対しては非 null アサーションや型ガードを使うことで、「endpoint は必ず resolve されているべき」という意図を保ちつつ、テスト本体のカスタムエラー処理を取り除けます。
Suggested change
| expect(s3.config.forcePathStyle).toBe(true) | |
| const resolvedEndpoint = await s3.config.endpoint() | |
| const resolvedEndpoint = await s3.config.endpoint?.() | |
| if (!resolvedEndpoint) { | |
| throw new Error("endpoint should be resolved") | |
| } | |
| const url = new URL(endpoint) | |
| expect(resolvedEndpoint.hostname).toBe(url.hostname) | |
| expect(resolvedEndpoint.protocol).toBe(url.protocol) | |
| expect(s3.config.forcePathStyle).toBe(true) | |
| const resolvedEndpoint = await s3.config.endpoint?.() | |
| expect(resolvedEndpoint).toBeDefined() | |
| const url = new URL(endpoint) | |
| expect(resolvedEndpoint!.hostname).toBe(url.hostname) | |
| expect(resolvedEndpoint!.protocol).toBe(url.protocol) |
Original comment in English
suggestion (testing): Use test assertions instead of throwing to express the expectation that endpoint is defined
Since the failure condition here is part of the test’s expectation, it’s clearer and more idiomatic to assert on it rather than manually throwing. For example, expect(resolvedEndpoint).toBeDefined() followed by a non-null assertion or type guard for later property checks preserves the intent that the endpoint must be resolved and removes the custom error handling in the test body.
Suggested change
| expect(s3.config.forcePathStyle).toBe(true) | |
| const resolvedEndpoint = await s3.config.endpoint() | |
| const resolvedEndpoint = await s3.config.endpoint?.() | |
| if (!resolvedEndpoint) { | |
| throw new Error("endpoint should be resolved") | |
| } | |
| const url = new URL(endpoint) | |
| expect(resolvedEndpoint.hostname).toBe(url.hostname) | |
| expect(resolvedEndpoint.protocol).toBe(url.protocol) | |
| expect(s3.config.forcePathStyle).toBe(true) | |
| const resolvedEndpoint = await s3.config.endpoint?.() | |
| expect(resolvedEndpoint).toBeDefined() | |
| const url = new URL(endpoint) | |
| expect(resolvedEndpoint!.hostname).toBe(url.hostname) | |
| expect(resolvedEndpoint!.protocol).toBe(url.protocol) |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
TypeScript 7 no longer exposes the compiler API that `next build` relied on, so the "Running TypeScript" step failed with "TypeScript 7.0.2 does not provide the compiler API required by Next.js" and broke the Playwright webServer build. - Enable `experimental.useTypeScriptCli` so Next.js type-checks via the TypeScript CLI (ref JamBalaya56562/blog#1076). - Fix type errors newly surfaced by the CLI in unit tests: - type mock parameters so recorded call tuples are non-empty - cast through `unknown` for incompatible structural casts - guard possibly-undefined endpoint providers and object spreads Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
dependencies
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TypeScript 7 no longer exposes the compiler API that
next buildrelied on, so the "Running TypeScript" step failed with
"TypeScript 7.0.2 does not provide the compiler API required by
Next.js" and broke the Playwright webServer build.
experimental.useTypeScriptCliso Next.js type-checks viathe TypeScript CLI (ref fix: restore production build under TypeScript 7 JamBalaya56562/blog#1076).
unknownfor incompatible structural castsCo-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Summary by Sourcery
TypeScript 7 向けに Next.js TypeScript CLI の型チェックを有効化し、より厳密な型安全性に対応するようテストを更新します。
New Features:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
Enable Next.js TypeScript CLI type-checking for TypeScript 7 and update tests for stricter type safety.
New Features:
Enhancements:
Tests: