Vendor find-my-way router - #7013
Conversation
🦋 Changeset detectedLatest commit: 23582f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This PR vendors the find-my-way-ts@0.1.6 radix router and its fast-querystring parser into the effect package, replacing the external dependency. I reviewed the public API surface, the router implementation, the query-string parser, the test port, and the consumer integration in HttpRouter.ts.
- Dependency removal:
find-my-way-tsis removed frompackages/effect/package.jsonandpnpm-lock.yaml; no remaining references exist in the repo. - Public API:
FindMyWay.tsexportsRouterConfig,PathInput,Router,FindResult, andmake, matching the shape previously provided by the dependency. - Runtime behavior: the vendored router preserves the defaults and matching semantics used by
HttpRouter.makeandasHttpEffect, including trailing-slash / duplicate-slash normalization and HEAD→GET fallback. - Tests: seven upstream router test files are ported and pass;
HttpClient.test.tsandMcpServer.test.tsalso pass. - Validation run:
pnpm install --frozen-lockfile,pnpm lint-fix,pnpm check, targetedFindMyWaytests,HttpClient.test.ts, andMcpServer.test.tsall pass.
One pre-existing upstream limitation was noted: Router.has() returns false even for registered routes, identical to find-my-way-ts@0.1.6. HttpRouter uses find() exclusively, so this is not a regression.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
7a0966b to
eb9fc9f
Compare
There was a problem hiding this comment.
Important
The incremental changes port several upstream router fixes and add good regression tests, but they also introduce or leave incomplete prototype-safety and decoding issues that should be addressed before merging.
Reviewed changes
This re-review covered the second commit (7a0966b0da) on top of the previously approved vendoring commit (fea3231f1c). The new work added a patch changeset, moved trees to a null-prototype object, added QUERY to the method list, and ported upstream fixes for optional root params, multi-parameter separator matching, regex/backtracking fallback, and maxParamLength fallback.
- Changeset added:
.changeset/fuzzy-routers-smile.mddescribes the patch-level router fixes. - Prototype-safe method lookup:
treesis nowObject.create(null), and a test confirmsrouter.find("constructor", "/")returnsundefined. QUERYmethod: added tohttpMethodssorouter.all("/all", ...)registers it.- Upstream bug fixes ported: optional parameters at
/, multi-param separator backtracking, regex mismatch / empty-capture /maxParamLengthfallback viabrothersNodesStack, and empty-segment regex matching. - Regression tests added: new cases in
methods.test.ts,optional-params.test.ts,path-params-match.test.ts, andregex.test.tscover the ported fixes.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
The latest commit addresses the licence placement feedback, but the previously raised prototype-safety, decoding, and regex concerns remain unaddressed.
Reviewed changes
The new commit (eb9fc9f2d1) only moved the MIT licence block below the imports in packages/effect/src/unstable/http/FindMyWay.ts, satisfying the earlier request from tim-smart. It also reconciled packages/effect/package.json and pnpm-lock.yaml against main (no net new dependencies introduced by this PR). No router implementation or test code changed.
- Licence placement: fixed — the module JSDoc now sits above the imports and the licence block follows.
- Remaining open items: the Pullfrog-originated threads on
staticChildrenprototype safety,compileCreateParamsparameter loss for__proto__, out-of-bounds decoding insafeDecodeURIComponent, and the ReDoS posture of the new multi-parameter regex still apply to the current head.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
|
@pullfrog Please review the latest changes addressing the open threads. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This re-review covered the new commit (23582f3a0) since the previous Pullfrog review (eb9fc9f2d1). The incremental work addresses every open Pullfrog-originated thread from the prior review.
- Prototype-safe static children and params:
ParentNode.staticChildrenand the object created bycompileCreateParamsnow both useObject.create(null), closing the__proto__/constructorpollution and parameter-loss vectors. New tests confirm/:__proto__preserves its value and inheritedObject.prototypeproperties no longer shadow registered static children. - Truncated percent encoding handling:
safeDecodeURIComponentnow breaks out of the decode loop when a%is followed by fewer than two characters, so paths like/test/a%correctly returnundefinedinstead of producing"undefined"substrings. - Backtracking regression coverage: a mixed-separator
/:foo-:bar-test witha-/binput was added alongside the existing 16k-separator test, both guarded by a 1s timeout. - Validation: all
FindMyWaytests (57),HttpClient.test.ts,McpServer.test.ts, andpnpm checkpass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers the latest commit (23582f3) which addresses the open Pullfrog threads from the prior review. All five previously raised concerns are resolved.
- Licence placement: the MIT licence block in
packages/effect/src/unstable/http/FindMyWay.tsnow sits below the imports, keeping the module JSDoc attached to the export. - Prototype-safe static children:
ParentNode.staticChildrenis initialized withObject.create(null), preventing inheritedObject.prototypekeys from affecting route lookup. - Prototype-safe params object:
compileCreateParamsnow builds the parameters record withObject.create(null), so parameter names such as__proto__are preserved. - ReDoS-safe multi-parameter matching: the regex for static parameter separators was changed from a greedy negative-lookahead to a bounded alternation with a lazy capture, and a mixed-separator regression test was added.
- Truncated percent encoding:
safeDecodeURIComponentnow bounds-checks before readingi + 1andi + 2, returningundefinedfor malformed trailing%sequences. - Regression coverage: new/updated tests confirm
__proto__parameter preservation, inherited static-child isolation, truncated percent encodings, and backtracking-safe separators.
Validation run on this branch: targeted FindMyWay tests pass and pnpm check succeeds.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
|
This run was cancelled 🛑 The workflow was cancelled before completion. Please check the link below for details. |
1 participant
Summary
find-my-way-tsv0.1.6 radix router and query-string parser under Effect'sFindMyWaymoduleeffectand the pnpm lockfileUpstream audit
The TypeScript rewrite forked from
delvedor/find-my-wayv7.7.0. Applicable fixes through current upstream were ported for:QUERYHTTP methodmaxParamLengthfallbackConstraint routing,
off/findRoute, and pretty-print fixes do not apply to this reduced API. The later semicolon-delimiter option was not included because it is an opt-in behavior/API change rather than a bug fix.Validation
pnpm lint-fixFindMyWaytest files (53 tests)packages/effect/test/HttpClient.test.tsandpackages/effect/test/unstable/ai/McpServer/McpServer.test.ts(24 tests)pnpm checkpnpm install --frozen-lockfileCloses EFF-451