fix: stop async object validation early when abortEarly is enabled#1417
fix: stop async object validation early when abortEarly is enabled#1417yslpn wants to merge 3 commits intoopen-circle:mainfrom
Conversation
|
@yslpn is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
the
size:XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors async object schema parsers (object, looseObject, objectWithRest, strictObject) to split per-entry work into parseEntry and processEntry helpers, enabling sequential validation with early abort when abortEarly is true while preserving parallel behavior when false; adds tests that assert abortEarly sequential behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Process each rest entry of schema if necessary | ||
| if (!dataset.issues || !config.abortEarly) { | ||
| for (const restDataset of restDatasets) { | ||
| processRestEntry(restDataset); | ||
| } |
There was a problem hiding this comment.
In the else branch (which is only entered when config.abortEarly is falsy), the condition !dataset.issues || !config.abortEarly on line 360 is always true because config.abortEarly is guaranteed to be falsy here. The !config.abortEarly part is a dead/redundant condition that can be simplified to just !dataset.issues or removed entirely (always processing rest entries in the non-abort-early path).
| // Process each rest entry of schema if necessary | |
| if (!dataset.issues || !config.abortEarly) { | |
| for (const restDataset of restDatasets) { | |
| processRestEntry(restDataset); | |
| } | |
| // Process each rest entry of schema | |
| for (const restDataset of restDatasets) { | |
| processRestEntry(restDataset); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/src/schemas/object/objectAsync.ts (1)
233-248: Consider extracting shared helpers in a future refactor.The
parseEntryandprocessEntrypatterns are nearly identical acrossobjectAsync,looseObjectAsync,strictObjectAsync, andobjectWithRestAsync. While the current inline approach works correctly, extracting these into shared internal utilities (e.g.,_parseObjectEntry,_processObjectEntry) could reduce duplication.This is not blocking for the current PR as the implementation is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/schemas/object/objectAsync.ts` around lines 233 - 248, The code in objectAsync repeats inline parseEntry and processEntry logic also used in looseObjectAsync, strictObjectAsync, and objectWithRestAsync; extract these into shared internal helpers (e.g., _parseObjectEntry and _processObjectEntry) and replace the inline closures in each file with calls to those helpers to remove duplication while preserving behavior (keep async semantics and abortEarly branching in the callers like objectAsync). Ensure the new helpers accept the same inputs previously captured by the closures (entry tuple or parsed value and the config/context needed), return the same outputs/promises, and update all references in objectAsync, looseObjectAsync, strictObjectAsync, and objectWithRestAsync to call the helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@library/src/schemas/object/objectAsync.ts`:
- Around line 233-248: The code in objectAsync repeats inline parseEntry and
processEntry logic also used in looseObjectAsync, strictObjectAsync, and
objectWithRestAsync; extract these into shared internal helpers (e.g.,
_parseObjectEntry and _processObjectEntry) and replace the inline closures in
each file with calls to those helpers to remove duplication while preserving
behavior (keep async semantics and abortEarly branching in the callers like
objectAsync). Ensure the new helpers accept the same inputs previously captured
by the closures (entry tuple or parsed value and the config/context needed),
return the same outputs/promises, and update all references in objectAsync,
looseObjectAsync, strictObjectAsync, and objectWithRestAsync to call the
helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71a6a518-1de8-456a-be99-6f872d86d693
📒 Files selected for processing (8)
library/src/schemas/looseObject/looseObjectAsync.test.tslibrary/src/schemas/looseObject/looseObjectAsync.tslibrary/src/schemas/object/objectAsync.test.tslibrary/src/schemas/object/objectAsync.tslibrary/src/schemas/objectWithRest/objectWithRestAsync.test.tslibrary/src/schemas/objectWithRest/objectWithRestAsync.tslibrary/src/schemas/strictObject/strictObjectAsync.test.tslibrary/src/schemas/strictObject/strictObjectAsync.ts
|
Thank you for creating this PR. Is there any benefits of integrating and providing an |
fabian-hiller
added
the
next version
|
Yes, there are existing issues and PRs about async validation improvements. Here's a summary: AbortController Support Early Abort Optimization (this PR) These two are complementary:
Other Related Discussions
Currently, the codebase has no AbortController support — PR #1203 would add it. Together with this PR, they'd provide comprehensive async validation control. To reply, just mention @dosu. How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other
|
Yes, I think This PR makes That also makes me think this PR is still valuable on its own for v1, while broader cancellation support can be explored separately for v2 |
|
Exactly right — your PR prevents starting unnecessary work, while AbortController would cancel work already in progress. They solve different parts of the problem and would work well together. For reference, PR #1203 already implements AbortController support by adding a To reply, just mention @dosu. How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other
|
|
If someone sets |
That's true. But we need to fix this. There are only two options:
The first option can be considered a breaking change. Which option do you prefer? |
|
Wouldn't it be best to validate in parallel but abort via AbortSignal on the first error we receive? |
That would be great, but here's what I'm thinking. AbortSignal only works where the specific API supports it. It can't cancel "any already running Promise." If async-check simply |
PR Description
This PR improves
abortEarlybehavior for asynchronous object schemas.Previously,
objectAsync,strictObjectAsync,looseObjectAsync, andobjectWithRestAsyncstarted validating all async entries in parallel before stopping on the first issue. That meantabortEarly: truestill paid the cost of starting later async validators.With this change:
abortEarly: trueSummary by CodeRabbit
Bug Fixes
Tests
Refactor