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

[Docs] Indicate create*Query options.results is a Doc[] #546

Merged
merged 2 commits into from
Apr 12, 2022

Conversation

ericyhwang
Copy link
Contributor

The Connection's createFetchQuery and createSubscribeQuery take an optional existing result array as options.results.

It should be an array of Doc instances:

// If present, the callback should have the signature function(error, results, extra)
// where results is a list of Doc objects.
Connection.prototype.createSubscribeQuery = function(collection, q, options, callback) {
return this._createQuery('qs', collection, q, options, callback);
};

This PR updates the docs to indicate the specific type, instead of Object[].

The `Connection`'s `createFetchQuery` and `createSubscribeQuery` take an optional existing result array as `options.results`.

It should be an array of Doc instances:
https://github.com/share/sharedb/blob/1cca122c63329665ebfdeceb1984921badb0cf4d/lib/client/connection.js#L563-L567

This PR updates the docs to indicate the specific type, instead of `Object[]`.
@coveralls
Copy link

coveralls commented Mar 1, 2022

Coverage Status

Coverage decreased (-97.4%) to 0.0% when pulling c49ad21 on docs-createQuery-options-types into 1cca122 on master.

Copy link
Contributor

@curran curran left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -105,7 +105,7 @@ Optional

> Default: `{}`

> `options.results` -- Object[]
> `options.results` -- [`Doc`]({{ site.baseurl }}{% link api/doc.md %})[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice! It might help to explicitly state in the docs what is expected with regards to subscription status.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree — might be nice to expand on how to actually get this Doc array (I have to admit I've never done this, so when I wrote the documentation, I didn't add any details)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! I'm (hopefully) going to be experimenting with this API soon, so I can test out what happens in the various cases and report back / update the docs when I do:

  • Doc not fetched or subscribed
  • Doc fetched but not subscribed
  • Doc fetched and subscribed

Copy link
Contributor

Choose a reason for hiding this comment

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

I have verified the behavior for the various cases.

Short answer: any combination of fetch and subscribe states appears to work, as long as each doc in results is initialized using connection.get.

Long answer:

  • If the docs in results are initialized with connection.get followed by ingestSnapshot (without subscribing), then the query implementation will subscribe to all of them (note that you still need to manually add the listeners for doc.on('load') and doc.on('op') - the subscribeQuery does not fire events when the individual docs are updated, only when the query result list is changed). In this case, we get a message that looks like this over WebSockets a: [["id1", 10], ["id2", 3],.... This does not contain the full documents, only their ids and versions, which is the expected behavior here. Awesome!
  • If the docs in results are initialized with only connection.get and nothing else, then a message appears overWebSockets that contains the full document contents for each individual doc.

@curran
Copy link
Contributor

curran commented Mar 19, 2022

Closes #501

(just commenting so we can easily find this PR from the issue)

Copy link
Contributor

@curran curran left a comment

Choose a reason for hiding this comment

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

Added suggestions that add more detail around expected state of the Doc instances.

docs/api/connection.md Outdated Show resolved Hide resolved
docs/api/connection.md Outdated Show resolved Hide resolved
Co-authored-by: Curran Kelleher <68416+curran@users.noreply.github.com>
@alecgibson alecgibson merged commit 92479d2 into master Apr 12, 2022
@alecgibson alecgibson deleted the docs-createQuery-options-types branch April 12, 2022 17:00
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.

4 participants