perf(core): skip generated-file writes when the content is unchanged - #3824
perf(core): skip generated-file writes when the content is unchanged#3824luantaraschi wants to merge 3 commits into
Conversation
Regenerating with unchanged inputs rewrote every artifact, updating mtime across the output directory. That wakes file watchers, triggers rebuilds downstream and does filesystem work that produces no new bytes. The barrel writers already compare against the file on disk before writing (orval-labs#3756). writeGeneratedFile now does the same, which covers the schema, single-mode and split-mode writers that go through it. The comparison happens after trailing whitespace is stripped, so it is the final output that is compared, not the generator's raw string. A file whose content genuinely differs, including because of a version stamp in its header, is still rewritten. Refs orval-labs#3787
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesGenerated file writing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@aqeelat please review! |
@orval/angular
@orval/axios
@orval/core
@orval/effect
@orval/fetch
@orval/hono
@orval/mcp
@orval/mock
orval
@orval/query
@orval/solid-start
@orval/swr
@orval/zod
commit: |
aqeelat
left a comment
aqeelat
left a comment
There was a problem hiding this comment.
The helper is the right primitive, but this is not complete enough for #3787 yet.
-
Apply the same conditional-write behavior to every generated artifact.
write-zod-specs.tsstill writes directly withfs.outputFile, andwrite-specs.tsdoes the same for schema-index creation andextraFiles. Those outputs will continue to churn mtime. -
Replace
pathExists()followed byreadFile(). It adds an unnecessary syscall and has a check-then-read race if the file disappears between calls. Read once and treatENOENTas a missing file. -
Replace the timing-based mtime assertion with
utimes()to set a known past mtime before the no-op generation. The test should assert the invariant without relying on a delay. -
Add repeated-generation coverage for at least one direct Zod output and one
extraFilesoutput. -
Add coverage for
formatter: 'prettier'. Comparing raw generated bytes does not prevent the raw-write-then-format-write cycle when the final formatted bytes are unchanged. Please handle the no-hook Prettier path in memory before comparison, while preserving the existingwrite → hook → formatterordering when hooks are configured.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/writers/file.test.ts (1)
42-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover normalization after the transform.
This test verifies transformed-content comparison, but
formatdoes not add trailing whitespace. Add trailing spaces in the transformed output, then assert that the stored content is normalized and the second generation preservesmtime.Suggested regression coverage
const format = async (_filePath: string, content: string) => - content.replaceAll("'", '"'); + content.replace(";\n", "; \n").replaceAll("'", '"');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/writers/file.test.ts` around lines 42 - 59, Update the “compares transformed content before writing” test so its format transform adds trailing spaces to the transformed output, then assert the file stores the normalized content without those spaces and that the second generation still preserves the original mtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/orval/src/formatters/prettier.ts`:
- Around line 19-30: Update the formatter returned by the exported formatter
function to remove the directory-based configs cache and call
prettier.resolveConfig(filePath) once for each transformed file, preserving the
existing file-specific filepath: filePath argument passed to prettier.format.
---
Nitpick comments:
In `@packages/core/src/writers/file.test.ts`:
- Around line 42-59: Update the “compares transformed content before writing”
test so its format transform adds trailing spaces to the transformed output,
then assert the file stores the normalized content without those spaces and that
the second generation still preserves the original mtime.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a00641e2-aca2-4cac-8409-592f1f7c69ec
📒 Files selected for processing (10)
packages/core/src/writers/file.test.tspackages/core/src/writers/file.tspackages/orval/src/formatters/prettier.test.tspackages/orval/src/formatters/prettier.tspackages/orval/src/generate-spec.test.tspackages/orval/src/utils/barrel.tspackages/orval/src/write-specs.test.tspackages/orval/src/write-specs.tspackages/orval/src/write-zod-specs.test.tspackages/orval/src/write-zod-specs.ts
| const configs = new Map< | ||
| string, | ||
| Awaited<ReturnType<typeof prettier.resolveConfig>> | ||
| >(); | ||
|
|
||
| return async (filePath, content) => { | ||
| try { | ||
| const directory = path.dirname(filePath); | ||
| let config = configs.get(directory); | ||
| if (!configs.has(directory)) { | ||
| config = await prettier.resolveConfig(filePath); | ||
| configs.set(directory, config); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'prettier\.ts$' . | sed 's#^\./##' || true
echo "== relevant lines =="
file=$(fd 'prettier\.ts$' . | head -n1)
if [ -n "$file" ]; then
cat -n "$file" | sed -n '1,120p'
fi
echo "== search usage of createPrettierFileTransform =="
rg -n "createPrettierFileTransform|configs = new Map|resolveConfig\\(filePath\\)" -S .Repository: orval-labs/orval
Length of output: 5069
🌐 Web query:
Prettier CLI API resolveConfig file path overrides returns different config per file
💡 Result:
When using the Prettier CLI API, the prettier.resolveConfig(filePath) function is designed to resolve configuration on a per-file basis [1][2]. It works by starting at the directory of the provided filePath and searching up the file tree until a configuration file is found [1][3]. If you observe different configurations being returned for different files, this is the expected behavior when those files are located in different directory structures or have their own local configuration files (such as .prettierrc) [3][4]. To manage per-file configuration overrides effectively: 1. Use the overrides property in your configuration file: This allows you to specify different settings for certain file extensions, folders, or specific files within a single configuration file [3][5]. Because resolveConfig is file-aware, it will evaluate these overrides based on the filePath you pass to it [1][5]. 2. Verify the resolved configuration: If you need to debug which configuration is being applied, you can programmatically inspect the result of await prettier.resolveConfig(filePath) for different files in your script [1][2]. 3. Explicit configuration: If you do not want Prettier to search the file tree, you can bypass the automatic resolution for specific files by passing the path of a specific configuration file to the options.config property when calling resolveConfig [1][2]. The resolveConfig function automatically handles the application of overrides by checking the file path against the files patterns defined in your configuration [3][5]. It is not a bug that it returns different results for different files; rather, it is how Prettier ensures that project-specific or file-specific rules are correctly respected based on their location [4].
Citations:
- 1: https://prettier.io/docs/api
- 2: https://github.com/prettier/prettier/blob/main/docs/api.md
- 3: https://prettier.io/docs/configuration
- 4: Config overrides do not work for files in the parent directory prettier/prettier#13556
- 5: https://github.com/prettier/prettier/blob/main/docs/configuration.md
🌐 Web query:
site:prettier.io/docs/api resolveConfig config cache per file documentation
💡 Result:
The prettier.resolveConfig function in the Prettier API resolves the configuration for a specific file by searching from the file's directory upwards through the directory structure [1]. By default, Prettier caches configuration files and plugins to improve performance [1]. Regarding cache management: 1. Per-file Caching: Prettier internally caches configuration results. You can disable this behavior by setting the useCache option to false in the options object passed to resolveConfig [1]. 2. Manual Cache Clearing: Because configuration and plugin loading is cached, you may need to manually clear the cache if the file system structure changes (such as during editor integrations). The prettier.clearConfigCache function is provided for this purpose to flush the existing configuration cache [1]. For more details on these functions and options, you can refer to the official Prettier API documentation [1].
Citations:
🌐 Web query:
Prettier v3 prettier.resolveConfig accepts options config resolveConfigCache false documentation
💡 Result:
In Prettier v3, the correct option to bypass caching when using prettier.resolveConfig is useCache, not resolveConfigCache [1][2]. By setting options.useCache to false, all configuration caching will be bypassed [1][3]. The documentation for prettier.resolveConfig(fileUrlOrPath [, options]) explicitly states: "If options.useCache is false, all caching will be bypassed" [1][2]. Note that there is also an options.resolveConfig property (which defaults to true), but this is used to enable or disable the searching for a configuration file entirely, rather than controlling the cache [1][4]. If set to false, Prettier will not search for a configuration file at all, which can be useful if the function is only being used to check if a file is ignored [1][2].
Citations:
- 1: https://prettier.io/docs/api
- 2: https://github.com/prettier/prettier/blob/main/docs/api.md
- 3: https://prettier.io/docs/next/api
- 4: https://github.com/prettier/prettier/blob/cf7db350/website/versioned_docs/version-stable/api.md
Resolve Prettier config by file path instead of directory.
resolveConfig(filePath) can return different options for different files in the same directory when using overrides, but the directory key makes later files reuse the first file’s resolved config. Call prettier.resolveConfig(filePath) once per transformed file and keep filepath: filePath in prettier.format; Prettier caches config files internally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/orval/src/formatters/prettier.ts` around lines 19 - 30, Update the
formatter returned by the exported formatter function to remove the
directory-based configs cache and call prettier.resolveConfig(filePath) once for
each transformed file, preserving the existing file-specific filepath: filePath
argument passed to prettier.format.
|
Thanks for going through it properly, the list was clear enough to work from. All five are in, in
Locally the five touched files run 73 tests green. Full |
Addresses #3787 by extending behaviour that already exists in this repo rather than introducing a policy.
reconcileBarrelandreconcileZodBarrelinpackages/orval/src/utils/barrel.tsalready read the file and skip the write when the content matches, added for #3756 with the same reasoning about mtime churn.writeGeneratedFiledid not, so every artifact that goes through it was rewritten on each run. That is the inconsistency the issue describes.writeGeneratedFilenow compares before writing, which covers the schema, single-mode and split-mode writers.The comparison runs after trailing whitespace is stripped, so what is compared is the bytes that would land on disk rather than the generator's raw string. That matters: two runs can produce different source strings and identical output, and the naive comparison would miss it. One of the tests exercises exactly that case.
On the open question in the issue, default versus
onlyUpdateWhenDifferent: I made it the default, since that is what the barrels already do and a second option would leave two writers in the codebase disagreeing about the same thing. A file whose content genuinely differs, including from a version stamp in its header, is still rewritten, so this does not suppress real changes. Happy to put it behind a flag instead if you would rather preserve write events for consumers who depend on them.Three tests in
packages/core/src/writers/file.test.ts, a new file, covering creation with whitespace stripping, an unchanged rewrite leaving mtime alone, and a changed rewrite still landing. The mtime test fails on the commit before this change.packages/coregoes from 2272 to 2275 tests. The full package suite passes, including the#3756barrel idempotency tests inpackages/orvalthat exercise repeat generation.tsc --noEmitandvp fmt --checkare clean.Summary by CodeRabbit
Improvements
Bug Fixes
Tests