Skip to content

Feature/async#755

Open
thumptech wants to merge 5 commits intoMeteor-Community-Packages:feature/asyncfrom
thumptech:feature/async
Open

Feature/async#755
thumptech wants to merge 5 commits intoMeteor-Community-Packages:feature/asyncfrom
thumptech:feature/async

Conversation

@thumptech
Copy link
Copy Markdown

Just opening this as I'm actively working on it. It's not passing all tests yet

@jankapunkt
Copy link
Copy Markdown
Member

thank you @thumptech can you please add a quick description on how it differs from #752 or if it attempts to achieve the same, I would be glad to review and test, once ready

@thumptech
Copy link
Copy Markdown
Author

thank you @thumptech can you please add a quick description on how it differs from #752 or if it attempts to achieve the same, I would be glad to review and test, once ready

I just took that branch (feature/async) and started working on it. I needed custom validation to be async for my application so I've just added the changes to make it work. All validation calls need to use promises now, so this is totally breaking as is mentioned in #752, and I'll have to follow through the collection2 and probably autoform packages I am assuming.

As far as I can tell, it's trying to achieve the same goal, just with async custom validation functions added as a requirement; as I need to access other collections to do validation in my main application.

@jankapunkt
Copy link
Copy Markdown
Member

@thumptech thanks for clarification and yes, its assumed to be breaking. ping me if you need reviews

@thumptech
Copy link
Copy Markdown
Author

@jankapunkt I've moved onto updating my main application for now. The simple-schema, collection2 and autoform packages I have made PR for are working in that application. If I can get a list of requirements for getting this ready to be a release canditate or alpha test or whatever that would be great - I've never done a pull request properly before so I'm not really sure of all the requirements. I didn't want to end up with my own private forks of all of these packages as they seem to still be in common use as far as I can tell.

I'm guessing I need to clean this up enough to be a 3.0.0-rc1 version on atmosphere so that it can be pulled in by the collection2 5.0.0-rc1, get that done and then finally autoform 9.0.0-rc1 would be able to pass tests as it can pull packages from atmosphere for its bespoke test application.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR attempts to convert the simple-schema validation system from synchronous to asynchronous (async/await) operations. The changes span across core validation logic, test helpers, and numerous test files. The PR represents a major version bump from 2.0.0 to 3.0.0, indicating a breaking API change.

Changes:

  • Core validation methods (validate, doValidation) converted to async/await
  • All test helpers updated to async functions
  • Test files updated to use async/await patterns
  • Package dependencies updated with new testing libraries

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
package.js Version bump to 3.0.0, added chai/chai-as-promised dependencies, incorrect git URL
lib/doValidation.js Core validation converted to async with Promise.all for parallel validation
lib/ValidationContext.js validate method now async with try/catch error handling
lib/SimpleSchema.js validate and related methods converted to async
lib/testHelpers/*.js All test helpers converted to async functions
lib/*.tests.js All test files updated to use async/await patterns
lib/reactivity.tests.js Reactivity tests restructured but missing await calls

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@thumptech
Copy link
Copy Markdown
Author

Thanks for running that @jankapunkt. I'll fix that up and push the changes soon. That one about the await calls on the reactivity tests actually doesn't matter, as a we are looking for the reactive result, not the promise result, but I'd best appease the bot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants