feat: runtime baseUrl (optional imports) - #3179
Conversation
|
Thanks! It may take some time, but we will definitely check it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a runtime base URL option ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (generate)
participant Generator as Orval Generator
participant Files as Generated Client
participant Runtime as App Runtime
participant API as API Server
Dev->>Generator: run codegen with `baseUrl.runtime` + optional imports
Generator->>Files: emit client code with template-literal URL prefix ${<runtime expr>} and added imports
Runtime->>Files: evaluate runtime expression (e.g., process.env / import.meta.env)
Files->>API: perform fetch to resolved URL
API-->>Files: response
Files-->>Runtime: return response (with runtimeValidation / includeHttpResponseReturnType if enabled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/getters/route.ts (1)
26-30: Guard against already-wrapped runtime expressions to fail fast at config time.The
runtimeExpressionToUrlPrefixfunction (lines 26-30) unconditionally wraps input with${...}. If a user providesprocess.env.API_BASE_URL, it correctly produces${process.env.API_BASE_URL}; however, if they mistakenly pass${process.env.API_BASE_URL}, the output becomes${${process.env.API_BASE_URL}}, which is invalid syntax in the generated template literal.The type guard
isBaseUrlRuntimeonly validates structure and string type—it does not validate content. While JSDoc documentation warns users to pass bare expressions, the code currently relies on compliance without enforcement. Consider adding a guard to detect already-wrapped expressions and throw a clear configuration error.💡 Proposed fix
function runtimeExpressionToUrlPrefix(expression: string): string { const t = expression.trim(); if (!t) return ''; + if (t.startsWith('${') && t.endsWith('}')) { + throw new Error( + "Invalid output.baseUrl.runtime: provide a JavaScript expression without the `${...}` wrapper.", + ); + } return '${' + t + '}'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/route.ts` around lines 26 - 30, The runtimeExpressionToUrlPrefix function currently wraps any input in ${...} which creates invalid nested template syntax when callers already pass a wrapped expression; update runtimeExpressionToUrlPrefix to detect already-wrapped expressions (e.g., strings starting with '${' and ending with '}') and throw a clear configuration error message instead of returning a malformed value; reference runtimeExpressionToUrlPrefix (and optionally the isBaseUrlRuntime guard) so you add the check at the start of that function, validate input.trim(), and throw an Error with a helpful message instructing users to pass the bare expression (e.g., "runtime expression must not be wrapped with ${...}").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/getters/route.ts`:
- Around line 26-30: The runtimeExpressionToUrlPrefix function currently wraps
any input in ${...} which creates invalid nested template syntax when callers
already pass a wrapped expression; update runtimeExpressionToUrlPrefix to detect
already-wrapped expressions (e.g., strings starting with '${' and ending with
'}') and throw a clear configuration error message instead of returning a
malformed value; reference runtimeExpressionToUrlPrefix (and optionally the
isBaseUrlRuntime guard) so you add the check at the start of that function,
validate input.trim(), and throw an Error with a helpful message instructing
users to pass the bare expression (e.g., "runtime expression must not be wrapped
with ${...}").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1278b371-7955-4446-80bd-e18036fed5cb
📒 Files selected for processing (7)
docs/content/docs/guides/fetch.mdxdocs/content/docs/guides/set-base-url.mdxdocs/content/docs/reference/configuration/output.mdxpackages/core/src/getters/route.test.tspackages/core/src/getters/route.tspackages/core/src/types.tspackages/orval/src/client.ts
soartec-lab
left a comment
soartec-lab
left a comment
There was a problem hiding this comment.
@mikan3rd
Thank you for the great improvements. I left a few comments, so please check them out.
| }); | ||
| ``` | ||
|
|
||
| ### runtime |
There was a problem hiding this comment.
Please write the details for each parameter, nested within the document structure 👍
### runtime
#### baseUrl
#### imports
There was a problem hiding this comment.
Headings now follow config keys: ### runtime + #### runtime / #### imports, and ### getBaseUrlFromSpecification + #### variables / #### index. Anchors #runtime and #getbaseurlfromspecification kept.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/getters/route.test.ts (1)
148-172: Add one precedence regression case (runtimeshould win even whenserversexists)Nice coverage for runtime expression patterns. A small extra case would lock in branch precedence and prevent future regressions if
getFullRoutecondition order changes.Proposed test case addition
for (const [path, servers, config, expected] of [ @@ [ '/pets', undefined, { runtime: 'env.API_BASE_URL' }, '${env.API_BASE_URL}/pets', ], + [ + '/pets', + [{ url: 'https://from-spec.example.com' }], + { runtime: 'process.env.API_BASE_URL' }, + '${process.env.API_BASE_URL}/pets', + ], ['/path', undefined, { runtime: '' }, '/path'], ] as [string, OpenApiServerObject[] | undefined, BaseUrlRuntime, string][]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/route.test.ts` around lines 148 - 172, Add a test case in the existing table-driven tests for getFullRoute that ensures BaseUrlRuntime.runtime wins even when an OpenApiServerObject[] is provided: add an entry where path is '/pets', servers is a non-empty array (e.g., [{ url: 'https://api.example.com' }]), config.runtime is a runtime expression like 'process.env.API_BASE_URL', and expected is '${process.env.API_BASE_URL}/pets'; this ensures getFullRoute prioritizes the runtime value over servers.docs/content/docs/reference/configuration/output.mdx (1)
260-264: Consider adding a brief note about theGeneratorImportshape.Users unfamiliar with Orval's import configuration may benefit from knowing the basic structure of
GeneratorImport. Consider adding a brief inline reference or link to where this type is documented elsewhere (if applicable).📝 Optional enhancement
#### imports **Type:** `GeneratorImport[]` -Optional. When `runtime` references a symbol from another module, list the imports Orval should emit into generated clients. Paths are relative to the generated file, same idea as mutator imports. The `runtime` expression must be valid where the generated code runs (after those imports). +Optional. When `runtime` references a symbol from another module, list the imports Orval should emit into generated clients. Each entry accepts `name` (the symbol to import) and `importPath` (relative to the generated file, same idea as mutator imports). The `runtime` expression must be valid where the generated code runs (after those imports).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/reference/configuration/output.mdx` around lines 260 - 264, Add a short note describing the shape of GeneratorImport (its key properties and types) and/or a link to its type definition where the docs live; update the docs around the "imports" section to include a one-sentence summary of GeneratorImport (e.g., fields like "path", "names" or "default" and expected types) and add a "See also" link pointing to the GeneratorImport type declaration so users can view the full schema; reference the symbol name GeneratorImport in the text so readers know which type to look up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/content/docs/reference/configuration/output.mdx`:
- Around line 260-264: Add a short note describing the shape of GeneratorImport
(its key properties and types) and/or a link to its type definition where the
docs live; update the docs around the "imports" section to include a
one-sentence summary of GeneratorImport (e.g., fields like "path", "names" or
"default" and expected types) and add a "See also" link pointing to the
GeneratorImport type declaration so users can view the full schema; reference
the symbol name GeneratorImport in the text so readers know which type to look
up.
In `@packages/core/src/getters/route.test.ts`:
- Around line 148-172: Add a test case in the existing table-driven tests for
getFullRoute that ensures BaseUrlRuntime.runtime wins even when an
OpenApiServerObject[] is provided: add an entry where path is '/pets', servers
is a non-empty array (e.g., [{ url: 'https://api.example.com' }]),
config.runtime is a runtime expression like 'process.env.API_BASE_URL', and
expected is '${process.env.API_BASE_URL}/pets'; this ensures getFullRoute
prioritizes the runtime value over servers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc651c3c-fc09-4008-b307-bb59558c3551
📒 Files selected for processing (2)
docs/content/docs/reference/configuration/output.mdxpackages/core/src/getters/route.test.ts
soartec-lab
left a comment
soartec-lab
left a comment
There was a problem hiding this comment.
I left a small comment, please check it.
| ``` | ||
|
|
||
| ### getBaseUrlFromSpecification | ||
| ### Runtime URL expression {#runtime} |
There was a problem hiding this comment.
Match the configuration with other properties.
| ### Runtime URL expression {#runtime} | |
| ### baseUrl |
There was a problem hiding this comment.
Using ### runtime instead of ### baseUrl to avoid repeating ## baseUrl. Spec side is ### getBaseUrlFromSpecification for the same reason.
|
|
||
| Values for variables used in server URL templates from the OpenAPI `servers` field. | ||
|
|
||
| #### index {#index} |
There was a problem hiding this comment.
| #### index {#index} | |
| #### index |
There was a problem hiding this comment.
Removed {#index} from the #### index heading.
- BaseUrlRuntime (expression + optional imports), getFullRoute, merge imports in client generator - Docs (output reference, fetch/set-base-url) and route tests
7801993 to
2024c89
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/getters/route.ts (1)
83-85: Minor: trailing slash normalization doesn't apply to runtime expressions.The slash deduplication logic at lines 125-127 checks
base.endsWith('/')at codegen time. For runtime expressions,baseis literally${process.env.API_BASE_URL}(ending with}), so this check never triggers. At runtime, if the environment variable ends with/and the route starts with/, the generated URL will have double slashes.This is an inherent limitation of runtime expressions. Consider documenting that users should ensure their runtime base URL values don't include a trailing slash, or alternatively, apply normalization at runtime:
♻️ Optional: runtime slash normalization
- if (isBaseUrlRuntime(baseUrl)) { - return runtimeExpressionToUrlPrefix(baseUrl.runtime); - } + if (isBaseUrlRuntime(baseUrl)) { + // Trim trailing slash at runtime to avoid double slashes + return runtimeExpressionToUrlPrefix(`(${baseUrl.runtime}).replace(/\\/$/, '')`); + }However, this adds complexity and the simpler approach is documenting the expectation. Your call.
Also applies to: 124-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/route.ts` around lines 83 - 85, The runtime base URL case isn't normalized because the compile-time check uses base.endsWith('/') on a template expression; update the runtime path by trimming a trailing slash at runtime when generating the URL (or alternatively add a clear doc comment) — locate the isBaseUrlRuntime(baseUrl) branch and the runtimeExpressionToUrlPrefix usage in getRoute/getters/route.ts, and either: 1) change the generated runtime code to perform a runtime trim (e.g., check the resolved base string for a trailing '/' and remove it before concatenating with the route), or 2) add a concise developer/user-facing comment near isBaseUrlRuntime/runtimeExpressionToUrlPrefix explaining that runtime-provided base URLs must not include a trailing slash and document this expectation. Ensure the chosen fix addresses both occurrences where base.endsWith('/') logic is currently applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/getters/route.ts`:
- Around line 83-85: The runtime base URL case isn't normalized because the
compile-time check uses base.endsWith('/') on a template expression; update the
runtime path by trimming a trailing slash at runtime when generating the URL (or
alternatively add a clear doc comment) — locate the isBaseUrlRuntime(baseUrl)
branch and the runtimeExpressionToUrlPrefix usage in getRoute/getters/route.ts,
and either: 1) change the generated runtime code to perform a runtime trim
(e.g., check the resolved base string for a trailing '/' and remove it before
concatenating with the route), or 2) add a concise developer/user-facing comment
near isBaseUrlRuntime/runtimeExpressionToUrlPrefix explaining that
runtime-provided base URLs must not include a trailing slash and document this
expectation. Ensure the chosen fix addresses both occurrences where
base.endsWith('/') logic is currently applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba8df3ad-af52-4a54-8b11-b03f55adcf2c
📒 Files selected for processing (7)
docs/content/docs/guides/fetch.mdxdocs/content/docs/guides/set-base-url.mdxdocs/content/docs/reference/configuration/output.mdxpackages/core/src/getters/route.test.tspackages/core/src/getters/route.tspackages/core/src/types.tspackages/orval/src/client.ts
✅ Files skipped from review due to trivial changes (3)
- packages/orval/src/client.ts
- docs/content/docs/guides/set-base-url.mdx
- docs/content/docs/guides/fetch.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/getters/route.test.ts
- docs/content/docs/reference/configuration/output.mdx
- packages/core/src/types.ts
|
@soartec-lab |
3 participants
Approach
Adds
baseUrl: { runtime, imports? }so generated clients embed a runtime expression in URL template literals (Issue #3071). This avoids resolving the API host at codegen time and keeps the built-in fetch stack (runtimeValidation,includeHttpResponseReturnType, etc.) without a custom mutator.Optional
importsreusesGeneratorImportso bindings from other modules are emitted into generated files alongside the URL.Caveats
importPathis relative to the generated client file (same idea as mutator paths).mock.baseUrl, separate fromoutput.baseUrl.Closes #3071
Summary by CodeRabbit
New Features
Documentation
Tests
Chore