Copilot review mirror for orval-labs/orval#3715 (issue #3713) — do not merge#7
Open
the-ult wants to merge 2 commits into
Open
Copilot review mirror for orval-labs/orval#3715 (issue #3713) — do not merge#7the-ult wants to merge 2 commits into
the-ult wants to merge 2 commits into
Conversation
…content-type resources The multi-content-type httpResource template built the request descriptor (path/query/version/body signal reads, normalizeRequest, Accept-header merge) eagerly, before httpResource() was called, and the factory only spread the pre-resolved object. Angular's httpResource tracks only signal reads that happen during factory execution, so these resources fetched once at creation and never refetched when their input signals changed. The request construction now lives in a buildRequest closure passed as the factory, matching the single-content-type output; accept-based branch selection stays eager. Also guards withSignal's default-detection against core's `default: false` boolean sentinel for query/body/header props, which wrongly rendered spec-required Signal parameters as optional (and produced invalid TS1016 overloads when combined with a trailing required accept parameter). Required query-param signals are now correctly non-optional — a compile-time tightening for consumers whose specs mark query params required. Fixes orval-labs#3713 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Angular httpResource code generator to ensure multi-content (content-negotiated) resources remain reactive by reading all signal inputs inside the httpResource request factory, addressing the behavior described in issue orval-labs#3713.
Changes:
- Refactors generated multi-content
httpResourcefunctions to pass abuildRequestfactory directly intohttpResource/.text/.arrayBufferbranches. - Adds regression tests (unit + integration) to verify signal reads occur within the factory and that resources refetch when input signals change.
- Adds a dedicated test config + snapshots for a multi-content query-params scenario and updates docs to clarify reactivity and
acceptsemantics.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/configs/angular.config.ts | Adds a new Angular generation config for the multi-content httpResource fixture. |
| tests/api-generation.spec.ts | Adds a regression test asserting generated multi-content resources read signals inside the httpResource factory. |
| tests/snapshots/angular/url-encode-parameters-http-resource/endpoints.ts | Snapshot updates reflecting updated generated httpResource signatures/output. |
| tests/snapshots/angular/issue-3624/petstore.client.service.ts | Snapshot updates reflecting updated generated httpResource signatures/output. |
| tests/snapshots/angular/http-resource-zod/endpoints.ts | Snapshot updates reflecting updated generated httpResource signatures/output. |
| tests/snapshots/angular/http-resource-zod-disabled/endpoints.ts | Snapshot updates reflecting updated generated httpResource signatures/output. |
| tests/snapshots/angular/http-resource-tags/pets.ts | Snapshot updates reflecting updated generated httpResource signatures/output. |
| tests/snapshots/angular/http-resource-multi-content/model/listItemsParams.ts | New snapshot model output for the multi-content fixture. |
| tests/snapshots/angular/http-resource-multi-content/model/items.ts | New snapshot model output for the multi-content fixture. |
| tests/snapshots/angular/http-resource-multi-content/model/item.ts | New snapshot model output for the multi-content fixture. |
| tests/snapshots/angular/http-resource-multi-content/model/index.ts | New snapshot model barrel for the multi-content fixture. |
| tests/snapshots/angular/http-resource-multi-content/model/error.ts | New snapshot model output for the multi-content fixture. |
| tests/snapshots/angular/http-resource-multi-content/model/confirmReservationBody.ts | New snapshot model output for the multi-content fixture. |
| tests/snapshots/angular/http-resource-multi-content/endpoints.ts | New snapshot endpoints output demonstrating the buildRequest factory pattern for multi-content httpResource. |
| samples/angular-app/src/app/http-resource.page.spec.ts | Adds integration coverage that multi-content resources refetch when signal inputs change. |
| samples/angular-app/src/api/http-resource/pets/pets.service.ts | Updates generated sample output to use buildRequest for content-negotiated resources. |
| samples/angular-app/src/api/http-resource-zod/pets/pets.service.ts | Updates generated sample output to use buildRequest for content-negotiated resources (zod variant). |
| samples/angular-app/src/api/http-both/pets/pets.resource.ts | Updates generated sample output to use buildRequest for content-negotiated resources (both mode). |
| samples/angular-app/snapshots/api/http-resource/pets/pets.service.ts | Snapshot updates for the generated sample API output. |
| samples/angular-app/snapshots/api/http-resource-zod/pets/pets.service.ts | Snapshot updates for the generated sample API output (zod). |
| samples/angular-app/snapshots/api/http-both/pets/pets.resource.ts | Snapshot updates for the generated sample API output (both mode). |
| packages/angular/src/http-resource.ts | Generator changes: computes buildRequest and passes it into httpResource branches; fixes default detection for signal args. |
| packages/angular/src/http-resource.test.ts | Adds targeted unit tests to prevent regressions in multi-content httpResource reactivity behavior. |
| docs/content/docs/guides/angular.mdx | Documents the reactivity behavior for content-negotiated resources and clarifies accept usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ti-content resources The multi-content accept dispatch covered json/text/arrayBuffer but never selected the blob factory, even though blob overloads are generated and blobType participates in the fallback chain — accept values like image/png fell through to the default branch and created the wrong resource kind. Emit a conditional blob branch (accept startsWith image/ or includes blob), mirroring the arrayBuffer branch and getHttpResourceFactory priority. Addresses Copilot review on orval-labs#3715. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Mirror of orval-labs#3715 for requesting a Copilot review. Do not merge.