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

Use requestData resources in UserPreferences #1064

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthew-white
Copy link
Member

Right now, the UserPreferences class sends requests using the http object in container. However, the more typical pattern is for requests to be sent via a requestData resource. It wasn't easy before to do so in UserPreferences, but I've made some improvements to requestData to make that possible (#1050, #1051).

Right now, UserPreferences manually calls withAuth(). It also has to manage request cancelation. Those are both things that requestData already knows how to do.

This PR also removes navigator.locks, closing #1044.

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

We don't yet have automated tests around user preferences: see #1049. However, I'll do a quick check of the behavior before merging the PR.

@getodk/testers, could you please check the user preference functionality during regression testing? (Maybe you were planning to already.) This PR makes changes to the underlying code, so it'd be good to verify that everything still works as expected.

Why is this the best possible solution? Were any other approaches considered?

One reason maybe not to use requestData resources is that resources have reactive state, and nothing about the UserPreferences.prototype.#propagate() method requires reactivity. However, I think it's fine for there to be a little extra reactivity: I don't expect there to be performance problems due to that. Overall, I think it's clearer to reuse the typical pattern of using requestData resources to send requests.

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

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.

1 participant