Removing DependsOnMethod
#2938
Removing DependsOnMethod
#2938
Conversation
RobinTail
added
refactoring
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks✅ Passed checks (3 passed)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
express-zod-api/src/endpoint.ts (1)
148-151: Bug: Object.freeze(undefined) will throw when methods are not setIf an endpoint is built without explicit methods, accessing
methodsthrows a TypeError. Returnundefinedinstead when absent.- public override get methods() { - return Object.freeze(this.#def.methods); - } + public override get methods() { + return this.#def.methods + ? (Object.freeze(this.#def.methods) as ReadonlyArray<Method>) + : undefined; + }
🧹 Nitpick comments (8)
express-zod-api/src/endpoints-factory.ts (1)
55-56: Nit: tighten JSDoc wording for clarity.“defined by Routing” can read as behavior-driven; suggest “defined in Routing keys” to point at the declaration site.
- * @default "get" unless method is explicitly defined by Routing + * @default "get" unless method is explicitly defined in Routing keysCHANGELOG.md (1)
7-19: v26 entry looks good; add one migration hint re HEAD.Many users relied on implicit HEAD with GET. A one-line note here will reduce confusion.
### v26.0.0 - `DependsOnMethod` removed: use flat syntax with explicit method and a slash; + - Note: HEAD remains auto-handled for endpoints that support GET; no `"head /"` mapping is required.migration/index.ts (1)
88-91: Tiny consistency nit: keep trailing commas in fallback branch.Add a trailing comma after the manual-migration comment for uniform diffs and to match outputs in tests.
- : `${ctx.sourceCode.getText(prop)}, /** @todo migrate manually */`; + : `${ctx.sourceCode.getText(prop)}, /** @todo migrate manually */,`;migration/index.spec.ts (1)
20-68: Add test coverage: string-literal keys and chained.deprecated().nest()orders.These cases guard against regressions in the fixer and reflect how real code may appear.
tester.run("v26", migration.rules.v26, { valid: [`const routing = { "get /": someEndpoint };`], invalid: [ + { + name: "string-literal method key", + code: `const routing = new DependsOnMethod({ "get": someEndpoint });`, + output: `const routing = {\n"get /": someEndpoint,\n};`, + errors: [{ messageId: "change", data: { subject: "value", from: "new DependsOnMethod(...)", to: "its argument object and append its keys with ' /'" } }], + }, { name: "basic DependsOnMethod", code: `const routing = new DependsOnMethod({ get: someEndpoint });`, output: `const routing = {\n"get /": someEndpoint,\n};`, errors: [ { messageId: "change", data: { subject: "value", from: "new DependsOnMethod(...)", to: "its argument object and append its keys with ' /'", }, }, ], }, + { + name: "deprecated + nest (deprecated before nest)", + code: `const routing = new DependsOnMethod({ get: epA }).deprecated().nest({ some: epB });`, + output: `const routing = {\n"get /": epA.deprecated(),\n"some": epB,\n};`, + errors: [{ messageId: "change", data: { subject: "value", from: "new DependsOnMethod(...)", to: "its argument object and append its keys with ' /'" } }], + }, + { + name: "deprecated + nest (nest before deprecated)", + code: `const routing = new DependsOnMethod({ get: epA }).nest({ some: epB }).deprecated();`, + output: `const routing = {\n"get /": epA.deprecated(),\n"some": epB,\n};`, + errors: [{ messageId: "change", data: { subject: "value", from: "new DependsOnMethod(...)", to: "its argument object and append its keys with ' /'" } }], + }, { name: "deprecated DependsOnMethod", code: `const routing = new DependsOnMethod({ get: someEndpoint }).deprecated();`, output: `const routing = {\n"get /": someEndpoint.deprecated(),\n};`, errors: [ { messageId: "change", data: { subject: "value", from: "new DependsOnMethod(...)", to: "its argument object and append its keys with ' /'", }, }, ], }, { name: "DependsOnMethod with nesting", code: `const routing = new DependsOnMethod({ get: someEndpoint }).nest({ some: otherEndpoint });`, output: `const routing = {\n"get /": someEndpoint,\n"some": otherEndpoint,\n};`, errors: [ { messageId: "change", data: { subject: "value", from: "new DependsOnMethod(...)", to: "its argument object and append its keys with ' /'", }, }, ], }, ], });README.md (1)
301-306: LGTM: method-based routing example matches the new syntax.Clear illustration of
"METHOD /"keys under a path. Consider a side note that HEAD is auto-handled for GET (no"head /"needed).example/routing.ts (1)
1-1: Use a type-only import for RoutingTo avoid emitting a value import in JS builds (and to play nicely with TS “verbatimModuleSyntax”), import Routing as a type.
-import { Routing, ServeStatic } from "express-zod-api"; +import type { Routing } from "express-zod-api"; +import { ServeStatic } from "express-zod-api";express-zod-api/src/routing.ts (1)
7-7: Correct: type-only import for AbstractEndpointThis avoids runtime imports; matches the library’s typing style. While here, consider also marking OnEndpoint as a type-only import for consistency.
-import { OnEndpoint, walkRouting } from "./routing-walker"; +import type { OnEndpoint } from "./routing-walker"; +import { walkRouting } from "./routing-walker";express-zod-api/src/endpoint.ts (1)
44-49: Avoid mutating arguments in .nest()Object.assign mutates the provided routing object. Prefer returning a new object (keeps surprises low for callers).
- public nest(routing: Routing): Routing { - return Object.assign(routing, { "": this }); - } + public nest(routing: Routing): Routing { + return { "": this, ...routing }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
express-zod-api/tests/__snapshots__/index.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/routing.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)README.md(4 hunks)example/routing.ts(2 hunks)express-zod-api/src/depends-on-method.ts(0 hunks)express-zod-api/src/endpoint.ts(2 hunks)express-zod-api/src/endpoints-factory.ts(1 hunks)express-zod-api/src/index.ts(0 hunks)express-zod-api/src/routable.ts(0 hunks)express-zod-api/src/routing-walker.ts(0 hunks)express-zod-api/src/routing.ts(2 hunks)express-zod-api/tests/depends-on-method.spec.ts(0 hunks)express-zod-api/tests/endpoint.spec.ts(1 hunks)express-zod-api/tests/routable.spec.ts(0 hunks)express-zod-api/tests/routing.spec.ts(8 hunks)migration/index.spec.ts(1 hunks)migration/index.ts(2 hunks)
💤 Files with no reviewable changes (6)
- express-zod-api/tests/routable.spec.ts
- express-zod-api/tests/depends-on-method.spec.ts
- express-zod-api/src/routing-walker.ts
- express-zod-api/src/routable.ts
- express-zod-api/src/index.ts
- express-zod-api/src/depends-on-method.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-28T18:58:10.064Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2428
File: express-zod-api/src/index.ts:44-44
Timestamp: 2025-05-28T18:58:10.064Z
Learning: The type-only import `import type {} from "qs";` in express-zod-api/src/index.ts is necessary to avoid TS2742 errors for exported functions like attachRouting, makeRequestMock, testEndpoint, and testMiddleware that have types depending on types/qs. This import provides the reference TypeScript needs to infer portable type names.
Applied to files:
express-zod-api/src/routing.tsexample/routing.tsREADME.mdexpress-zod-api/tests/routing.spec.ts
📚 Learning: 2025-05-27T20:03:34.213Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: example/factories.ts:35-42
Timestamp: 2025-05-27T20:03:34.213Z
Learning: The `./example` directory in the express-zod-api repository contains demonstration code for educational purposes only, not intended for production use. Example code can make simplified assumptions for brevity and clarity, and should not be flagged for missing production-level error handling, security measures, or edge case handling.
Applied to files:
example/routing.tsREADME.md
📚 Learning: 2025-05-27T20:27:17.015Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: express-zod-api/src/json-schema-helpers.ts:1-3
Timestamp: 2025-05-27T20:27:17.015Z
Learning: Ramda is correctly listed as a dependency in express-zod-api/package.json, so imports of ramda utilities are properly supported.
Applied to files:
example/routing.ts
📚 Learning: 2025-05-28T07:58:09.853Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2546
File: express-zod-api/src/documentation-helpers.ts:508-512
Timestamp: 2025-05-28T07:58:09.853Z
Learning: In express-zod-api, when working with Zod's JSON schema override callbacks, using `delete` to mutate `ctx.jsonSchema` is the recommended approach per Zod's official documentation, even if it triggers performance linting warnings. This is preferable to creating copies with `undefined` values, especially for snapshot testing.
Applied to files:
README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (9)
README.md (2)
1178-1179: LGTM: deprecation API wording aligns with removal of DependsOnMethod.Accurately scopes
.deprecated()to schemas and Endpoint.
1182-1195: LGTM: updated routing deprecation example.Shows path-level deprecation via Endpoint instance without the removed wrapper.
express-zod-api/tests/endpoint.spec.ts (1)
134-147: Good coverage for the new .nest() APIThe assertions match the contract and validate both presence and shape. Consider also asserting that input isn’t mutated (or is, if that’s the intended contract) to lock the behavior.
If immutability is desired, add:
const input = { subpath: subject }; const result = subject.nest(input); expect(result).toEqual({ "": subject, subpath: subject }); expect(result).not.toBe(input); // immutabilityexample/routing.ts (1)
19-24: New per-method route syntax reads wellSwitch to
"patch /"under the:idnode is clear and aligns with the v26 routing shape.express-zod-api/tests/routing.spec.ts (4)
111-155: Nice: method-scoped assignments exercise the new shapeGood assertions on method registrations and a single OPTIONS route per path with CORS enabled.
156-182: Correct guard: endpoint must support assigned methodThe negative case ensures method/endpoint compatibility is enforced. Snapshot-based error check is appropriate here.
183-239: CORS header set includes all assigned methods (+HEAD)Solid regression for issue 705; verifies deterministic order and the custom header passthrough.
410-424: Good constraint: no nesting under explicitly-method routesPrevents ambiguous shapes early; error snapshot makes this future-proof.
express-zod-api/src/routing.ts (1)
21-21: Public API: DependsOnMethod removal — leftover references found; verify & updateRipgrep found occurrences in: migration/index.ts (lines 19, 81); migration/index.spec.ts (lines 24, 25, 32, 39, 40, 47, 54, 55, 62); and numerous entries in CHANGELOG.md.
Action: update migration/index.ts and migration/index.spec.ts to remove or adapt DependsOnMethod usage to the new flat routing syntax, or confirm these migrations intentionally target legacy code; keep CHANGELOG.md mentions only as historical notes if desired.
Due to #2938 This will set us free from the requirement to write slash to the keys when we imply method depending routing. Breaking change: `Integration` requires `config` parameter. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `methodLikeRouteBehavior` configuration option to control interpretation of method-like routing keys ("method" or "path") * Added `hasHeadMethod` option for endpoint documentation configuration * **Refactor** * Integration constructor now requires a `config` parameter during initialization * Routing syntax simplified to use cleaner method-based keys <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
It can now be replaced with
${method} /syntaxtodo
RoutableSummary by CodeRabbit
Breaking Changes
New Features
Documentation
Examples