WIP - make simple-schema fully Async#746
WIP - make simple-schema fully Async#746jankapunkt merged 6 commits intoMeteor-Community-Packages:feature/asyncfrom
Conversation
All errors are fixed expect some regarding lib/SimpleSchema.js and its class fields and static properties which are recents syntax updates
|
This is great 👍 I think this is great step forward. Let me check the next Days how it will behave with existing apps. |
Ensuring compatibility with Meteor-Community-Packages/meteor-simple-schema#746
|
Hi @jankapunkt , |
enabling compatibility with Meteor-Community-Packages/meteor-simple-schema#746
|
Hi @jankapunkt @vparpoil , Just wanted to check in on the status of this PR. We are looking to upgrade our app to Meteor version 3 and this change is crucial for us. Is there any chance the PR could be reviewed and merged soon? |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces async support for autoValue functions in SimpleSchema, making the clean() method async. The changes include full linting with standard conventions (semicolon removal), and adds test globals to package.json.
- Async support for
autoValuefunctions enabling denormalization scenarios - Full linting of the codebase with standardized formatting
- Updates to test configuration for proper linting coverage
Reviewed Changes
Copilot reviewed 76 out of 79 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/clean/setAutoValues.js | Converts setAutoValues function to async and updates loop structure for async autoValue execution |
| lib/clean/AutoValueRunner.js | Makes runForPosition method async to handle async autoValue functions |
| lib/clean/autoValue.tests.js | Updates test cases to use async/await pattern for clean() calls |
| lib/clean/defaultValue.tests.js | Updates test cases to use async/await pattern for clean() calls |
| tests/package.json | Adds describe and it to eslint globals |
| package.js | Removes semicolons following linting standards |
| Multiple utility files | Removes semicolons and adjusts formatting following linting standards |
Files not reviewed (1)
- tests/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
lib/clean/setAutoValues.js:42
- [nitpick] The variable name 'sortedItem' is not descriptive. Consider renaming it to 'autoValueFunction' or 'avFunction' to better indicate what it represents.
for (const sortedItem of sortedAutoValueFunctions) {
| for (const sortedItem of sortedAutoValueFunctions) { | ||
| const { func, fieldName, closestSubschemaFieldName } = sortedItem |
There was a problem hiding this comment.
[nitpick] This destructuring assignment would be clearer if the variable name in the previous line was more descriptive, as it would make this line more self-documenting.
| for (const sortedItem of sortedAutoValueFunctions) { | |
| const { func, fieldName, closestSubschemaFieldName } = sortedItem | |
| for (const autoValueFunction of sortedAutoValueFunctions) { | |
| const { func, fieldName, closestSubschemaFieldName } = autoValueFunction |
| const autoValue = await func.call({ | ||
| closestSubschemaFieldName: closestSubschemaFieldName.length ? closestSubschemaFieldName : null, | ||
| field(fName) { | ||
| return getFieldInfo(mongoObject, closestSubschemaFieldName + fName); | ||
| field (fName) { | ||
| return getFieldInfo(mongoObject, closestSubschemaFieldName + fName) | ||
| }, | ||
| isModifier, | ||
| isUpsert, | ||
| isSet: (value !== undefined), | ||
| key: affectedKey, | ||
| operator, | ||
| parentField() { | ||
| return parentFieldInfo; | ||
| parentField () { | ||
| return parentFieldInfo | ||
| }, | ||
| siblingField(fName) { | ||
| return getFieldInfo(mongoObject, fieldParentName + fName); | ||
| siblingField (fName) { | ||
| return getFieldInfo(mongoObject, fieldParentName + fName) | ||
| }, | ||
| unset() { | ||
| doUnset = true; | ||
| unset () { | ||
| doUnset = true | ||
| }, | ||
| value, | ||
| ...(extendedAutoValueContext || {}), | ||
| }, mongoObject.getObject()); | ||
| ...(extendedAutoValueContext || {}) | ||
| }, mongoObject.getObject()) | ||
|
|
There was a problem hiding this comment.
The autoValue function is now awaited, but there's no error handling for async functions that might throw. Consider adding try-catch to handle potential async errors gracefully.
Motivations
Following Meteor 3 compatibility PR, a vote has been casted and the community voted for a full async compatibility
From a quick investigation, here are the functions that could benefit of being async :
.clean()asyncautoValuelabel- my feeling is that using async function here is redundant withautoValue.validate()asynccustomrequiredandoptionalmin,max,minCount,maxCount,exclusiveMin,exclusiveMaxallowedValuesregExskipRegExCheckForEmptyStringsWhat's included
This PR introduces async support for
autoValue. Here's an example use case:This feature is particularly useful for denormalization scenarios, such as adding fields like
createdByNameto your data schema.Changes
autoValue. All existing tests continue to pass.meteor npm run lint:fix(as outlined in CONTRIBUTING.md)..clean()is now Async and must be awaitedcollection2to addawaitonclean()calls and benefit of asyncautoValuevalidator()is now Async and must be awaitedValidatedMethodsdeclarationsWhat's next
validate()async is too much of a breaking change for now. I suggest focusing onautoValueand maybelabelThank you for you input!