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

Make dataset resource reactive so that entity count is updated #913

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

matthew-white
Copy link
Member

We use dataset.entities to show the entity count. On the entity Data page, that allows us to show the entity count in the download button even before the OData response has been received. I don't think we show dataset.entities outside the Data page, but we could in the future. odataEntities is a resource local to the Data page, but dataset is available on all dataset pages.

The OData count is more likely to be up-to-date than dataset.entities: the dataset is requested before the OData and may have been requested some time ago. If the two counts differ, then dataset.entities is updated to match the OData count:

watchEffect(() => {
if (dataset.dataExists && odataEntities.dataExists &&
!odataEntities.filtered)
dataset.entities = odataEntities.count;
});

We do something similar with form.submissions and the OData count on the Submissions page, as well as with formDraft.get().submissions and the OData count on the Draft Testing page.

However, while looking into getodk/central#560, it seems clear that this mechanism isn't fully working. Backend can return an incorrect count for dataset.entities (getodk/central-backend#1062), yet even then, dataset.entities doesn't seem to be updated with the correct OData count.

Actually, it turns out that dataset.entities is updated, but that update isn't reflected in the DOM, because dataset isn't reactive. For performance reasons, resources have to opt into reactivity. I made dataset reactive, and after that, the count started being updated in the DOM.

What has been done to verify that this works as intended?

I tried it locally and wrote a new test.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Anytime something is made reactive, it could theoretically have performance implications. However, I can't think of anywhere where that would be an issue for dataset. I also made dataset shallow reactive rather than deeply reactive, which should help with performance.

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed two tests in this file. Previously, those tests didn't create a test dataset. I'm a little surprised that they passed! However, once I started using shallowReactive(), the tests started logging warnings about calling shallowReactive() on undefined. (Since there was no dataset, the response data was undefined.)

@matthew-white matthew-white merged commit abd09e4 into master Dec 13, 2023
1 check passed
@matthew-white matthew-white deleted the reactive-dataset branch December 13, 2023 01:21
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.

2 participants