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

[Meteor 3] Mongo.Cursor.observeChanges returns a Promise instead of a query handle #12918

Open
ebroder opened this issue Dec 13, 2023 · 9 comments

Comments

@ebroder
Copy link
Contributor

ebroder commented Dec 13, 2023

With the current alpha release of v3, calling observeChanges (at least on the server side - client might be different!) now returns a Promise<LiveQueryHandle>. Previously, it returned a LiveQueryHandle directly. This means that it's not possible to write code that works correctly on both v2 and v3.

Specifically, MongoConnection.prototype._observeChanges is now an async function, causing it to return a Promise).

It seems ideal, if possible, to continue returning a LiveQueryHandle directly rather than returning a Promise. However, if that's not possible, then I would request renaming observeChanges to observeChangesAsync and introducing a wrapper in v2 so that there's a clean migration path.

@ebroder
Copy link
Contributor Author

ebroder commented Dec 13, 2023

For the record I suspect that there's a similar issue in observe, but my code only uses observeChanges so I haven't verified that directly.

@radekmie
Copy link
Collaborator

These are definitely fine on the client (I just checked on Simple Tasks) and there's isReadyPromise to await if needed, so it's only a server-side problem.

@manueltimita
Copy link

I just noticed the same issue in meteor-publish-composite, which depends on observe. I'm about to open an issue there and suggest a pull request

@Grubba27
Copy link
Contributor

I just confirmed it; on the server, it does return a promise. The client is fine.

@champel
Copy link

champel commented Jan 30, 2024

The existing implementation of observe and observeChanges does not return until the initial fetch is complete. This enables the triggering of an action once the initial additions have been processed. However, if you solely return the LiveQueryHandler without the promise, it becomes impossible to detect the conclusion of the initial fetch. I would prefer a distinct implementation such as observeAsync or observeChangesAsync that returns the promise, enabling me to identify the completion of the initial fetch.

@ebroder
Copy link
Contributor Author

ebroder commented Jan 30, 2024

Yeah I think my main objection here is the API change. I'd really like to be able to have a project that can run under both Meteor 2 and Meteor 3. For that to work, I think that any of the APIs in Meteor 3 need to also be present in Meteor 2 with the same signatures. We can drop APIs from Meteor 2 that are deprecated (e.g. Meteor 2 has both Cursor.fetch and Cursor.fetchAsync, but Meteor 3 only has Cursor.fetchAsync), but having an API change is (IMO) a problem.

@champel
Copy link

champel commented Jan 30, 2024

The central concept here is that not returning a promise in version 3 will result in the inability to uphold the existing functionality (which involves tracking the completion of the initial fetch). While the inclusion of observerAsync may not be ideal, it facilitates a gradual migration and aligns with the rest of API modifications.

@ebroder
Copy link
Contributor Author

ebroder commented Apr 19, 2024

I somehow missed that observeAsync and observeChangesAsync had landed in both 3.0 and 2.x, so I need to play with the API some, but that seems great. However, it does seem that both are missing in the TypeScript declarations in mongo.d.ts.

StorytellerCZ added a commit that referenced this issue Apr 20, 2024
Added missing TS types for `observeAsync` and `observeChangesAsync` as pointed out in #12918 by @ebroder
@nachocodoner
Copy link
Member

Yes, observeAsync and observeChangesAsync were introduced in Meteor 3.x. In Meteor 2.x, it's not merged yet but is slated for release 2.16, expected this week. Here's the pull request.

Your point about TypeScript is valid. I'll include that. Thanks.

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

No branches or pull requests

7 participants