feat!: migrate to @msw/url#2678
Conversation
commit: |
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/utils/matching/normalizePath.ts (1)
13-21: Use function overloads instead of a generic return type fornormalizePath.The current signature
<P extends Path>(path: P): Pclaims the function returns the same type as the input. However,cleanUrl()transforms string inputs by removing query parameters and hashes. This breaks the parametricity contract—callers passing string literals like"/user"expect that literal type back, but they receive a normalized string instead. Overloads preserve thestringvsRegExpdistinction without making this false guarantee.Proposed fix
-export function normalizePath<P extends Path>(path: P, baseUrl?: string): P { +export function normalizePath(path: RegExp, baseUrl?: string): RegExp +export function normalizePath(path: string, baseUrl?: string): string +export function normalizePath(path: Path, baseUrl?: string): Path { // RegExp paths do not need normalization. if (path instanceof RegExp) { return path } const maybeAbsoluteUrl = getAbsoluteUrl(path, baseUrl) - return cleanUrl(maybeAbsoluteUrl) as P + return cleanUrl(maybeAbsoluteUrl) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utils/matching/normalizePath.ts` around lines 13 - 21, The function normalizePath currently uses a generic <P extends Path>(path: P): P which incorrectly promises to return the exact input type; replace this with explicit overloads: declare normalizePath(path: RegExp, baseUrl?: string): RegExp and normalizePath(path: string, baseUrl?: string): string, then keep a single implementation signature normalizePath(path: Path, baseUrl?: string): Path that performs the runtime logic (the RegExp instanceof check, call to getAbsoluteUrl, and return cleanUrl(maybeAbsoluteUrl) as Path). This preserves correct typing for RegExp vs string callers while keeping the existing implementation using getAbsoluteUrl and cleanUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/utils/matching/matchRequestUrl.test.ts`:
- Around line 4-71: The test suite removed coverage for wildcard-port and
baseUrl-relative string matching; add back tests in matchRequestUrl.test.ts that
call matchRequestUrl to assert the wildcard-port pattern (e.g., matching new
URL('http://localhost:1234') against the string 'http://localhost:*' yields
matches: true, params: {}), and a test that asserts a baseUrl-relative string
pattern (e.g., matching new URL('http://example.com/base/path') against a
relative pattern like '/base/path' or a baseUrl-relative string) returns
matches: true with empty params; place these alongside the existing RegExp and
string tests using the same matchRequestUrl invocation to ensure regression
coverage for wildcard ports and base-relative string patterns.
In `@src/core/utils/matching/matchRequestUrl.ts`:
- Around line 29-38: The RegExp branch in matchRequestUrl incorrectly uses
matchAll when pattern.flags includes('g'), causing false positives and corrupted
params; change the logic to always perform a single-match with a non-global
RegExp: construct or reuse a RegExp without the 'g' flag (e.g., new
RegExp(pattern.source, pattern.flags.replace('g',''))) and call
cleanUrl.match(...) once, set matches = match != null, and build params from
(match ?? []).slice(1) as before so capture groups map correctly; remove the
matchAll/flat handling and ensure you reference the existing pattern, match,
cleanUrl, and params variables in the matchRequestUrl function.
---
Nitpick comments:
In `@src/core/utils/matching/normalizePath.ts`:
- Around line 13-21: The function normalizePath currently uses a generic <P
extends Path>(path: P): P which incorrectly promises to return the exact input
type; replace this with explicit overloads: declare normalizePath(path: RegExp,
baseUrl?: string): RegExp and normalizePath(path: string, baseUrl?: string):
string, then keep a single implementation signature normalizePath(path: Path,
baseUrl?: string): Path that performs the runtime logic (the RegExp instanceof
check, call to getAbsoluteUrl, and return cleanUrl(maybeAbsoluteUrl) as Path).
This preserves correct typing for RegExp vs string callers while keeping the
existing implementation using getAbsoluteUrl and cleanUrl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ead8705b-60a3-4632-b0d4-66a175a86913
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.jsonsrc/core/utils/matching/matchRequestUrl.test.tssrc/core/utils/matching/matchRequestUrl.tssrc/core/utils/matching/normalizePath.tstest/node/rest-api/request/matching/all.node.test.tstest/node/rest-api/request/matching/path-params-optional.node.test.ts
URLPatternfor request matching #2209RegExpinstance for matching, listing those tokens. The mix isn't supported anymore.Changes
path-to-regexpto an in-house@msw/url. Path matching is no longer regexp-based but token-based. Better performance, fewer vulnerabilities.@msw/urlis faster thanpath-to-regexpin most scenarios, in others the performance is roughly the same).Todos
matchRequestUrl.test.ts. No need to assert whatmatchPatterndoes already. Actually test what the function does: handlesRegExp, cleans urls, normalizes the path, and 1-2 simple absolute/relative url matches for posterity.paramsvalue for matching againstRegExpinpath-to-regexp. If it returned index-based capturing groups, implement that.