Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for asynchronous mongo methods #443

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

klablink
Copy link

The implementation includes support for Meteor versions: 2.2, 2.13.3 and 3.0-alpha.15. This involved a considerable change to the code due to the fact that the three versions have a different way of handling remote collections. As expressed in the forum, we still have behaviour to clarify in version 3 in order to declare the work completed.

tests/package.json Outdated Show resolved Hide resolved
.github/workflows/testsuite.yml Show resolved Hide resolved
Copy link
Member

@jankapunkt jankapunkt left a comment

Choose a reason for hiding this comment

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

Generally approved. I only have requests for some documentation / comments.

@StorytellerCZ Once this is merged, I think we should do an RC before publishing the next version, wdyt?

if (isInsertType(methodName) && typeof args[1] !== 'function') args.splice(1, 1)
}

if (async && !Meteor.isFibersDisabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for late review here. @klablink could you please add one or two lines of comments and summarize what's happening here in this if block.

I also would like to know about the logic behind the if-clause, because Meteor.isAsyncDisabled might be gone at some day, right?

}
}

function _methodMutationAsync (methodName) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a short method-summary using jsDoc notation?

@dallman2
Copy link

Any progress on this? @klablink @jankapunkt currently, I cannot get this package to play nicely with Meteor 3.0 alpha 19...

@harryadel
Copy link
Member

I'll taking a look at this @dallman2, no worries

@dallman2
Copy link

@harryadel FYSA, I found a dependency issue while trying to make all this work with meteor 3. It seems that SimplSchema has moved to ES6 style imports. While looking into it, I found that this workaround was the generally accepted solution for working with SimplSchema (for the time being): https://github.com/jankapunkt/simple-schema-3

In the README for that repo, they state that the imports have to be updated to ES6 style (although I suspect there is a typo). Either way, I think that for collection2 to use SimplSchema, this line needs to use ES6 import syntax... I could be wrong though:)

This is the error that I am getting on startup currently on alpha 19.

W20231214-15:26:04.532(-8)? (STDERR) ReferenceError: SimpleSchema is not defined
W20231214-15:26:04.532(-8)? (STDERR)     at packages/aldeed_collection2.js:31:1
W20231214-15:26:04.532(-8)? (STDERR)     at packages/aldeed_collection2.js:568:4
W20231214-15:26:04.532(-8)? (STDERR)     at packages/aldeed_collection2.js:572:4
W20231214-15:26:04.533(-8)? (STDERR)     at load (packages/core-runtime.js:139:16)
W20231214-15:26:04.533(-8)? (STDERR)     at onDepLoaded (packages/core-runtime.js:118:7)
W20231214-15:26:04.533(-8)? (STDERR)     at packages/core-runtime.js:153:7
W20231214-15:26:04.533(-8)? (STDERR)     at Array.forEach (<anonymous>)
W20231214-15:26:04.533(-8)? (STDERR)     at packages/core-runtime.js:152:22
W20231214-15:26:04.533(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:179:14)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)
W20231214-15:26:04.534(-8)? (STDERR)     at evaluateNextModule (packages/core-runtime.js:218:7)

@jankapunkt
Copy link
Member

Yeah the situation with simple schema is really frustrating. I will look into this later today

@dallman2
Copy link

Yeah the situation with simple schema is really frustrating. I will look into this later today

The fix that I had suggested seemed to work, but I've ben working on other dependency issues, so I cannot verify that the complete correctness of the fix...

package/collection2/package.js Show resolved Hide resolved
tests/autoValue.tests.js Show resolved Hide resolved
expect(doc.serverAV).toBe(1);
});
it('runs function once', async function () {
const id = await callMongoMethod(collection, 'insert', [{}])
Copy link
Member

Choose a reason for hiding this comment

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

How can we ensure if it's backwards compatible if all the tests now use the async version? Shouldn't we keep the old sync test and also async ones?
This is a question really as I'm new to the async lands 😅

@harryadel harryadel self-assigned this Dec 20, 2023
@harryadel
Copy link
Member

@klablink I'm trying to push changes but it appears you've disabled the maintainers the ability to edit the PR, can you revert it?

@harryadel harryadel changed the base branch from master to meteor-3 December 20, 2023 13:39
@harryadel harryadel merged commit abc15c4 into Meteor-Community-Packages:meteor-3 Dec 20, 2023
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.

5 participants