-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add user preference persistence framework #1024
base: master
Are you sure you want to change the base?
Add user preference persistence framework #1024
Conversation
Part of the currentUser resource. Uses JS proxies to enable some — hopefully comfortable — abstractions for working with preferences.
src/request-data/resources.js
Outdated
_container, | ||
abortControllers: {}, | ||
instanceID: crypto.randomUUID(), | ||
propagate: (k, v, projectId) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthew-white the other week you mentioned you'd like to see the locking dance factored out, I'll have a shot at that soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I know I promised you some comments! I'll try to get those to you ASAP. I'm hoping that we can reuse some of our existing logic around sending and aborting requests in particular. And yeah, if there's a problem around locking, I suspect that will be relevant in places outside this code. It'd be awesome if you could file an issue describing the race condition you're thinking of, and then we can figure out how we want to address it (ideally in a generic way). I think the part I don't understand yet is why locking is necessary if we also have abort logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the part I don't understand yet is why locking is necessary if we also have abort logic.
The outer locking serves for the use case you'd expect when you see locking: to protect a critical section (or at least, it looks like a critical section to me, I'm used to reasoning with threads but these are coroutines, so... maybe not? I still need to dive into that.)
The inner locking serves as an easy bookkeeping way to determine whether there's another request (for the same grouping key) in flight. The nice thing is that a lock is guaranteed to be released (as soon as the request resolves or throws) with no further work from my side. So it saves me bookkeeping busywork-code.
I forgot to say that I started out trying to modify the existing aborter implementation - basically I envisioned I'd adapt it so I could call it with a grouping key for my requests (by default initialized to 'default'
, thus backwards compatible) and then use an object {grouping_key: aborter}
for bookkeeping of the aborters, as now you could have N rather than just 0 or 1.
I honestly tried ;-) but it resisted my attemps at malling it (not the code's fault, it's just made for a different use case — there's a big difference between 0|1 and N). And then I thought "hmmm this bookkeeping could be more care-free anyway using locks", plus I thought I saw a race condition, ... and here we are.
When we have time we should walk through it together, that'd probably give the best mutual insights!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still haven't had a chance to write up my idea about reusing existing request logic, but I had a few unrelated comments that I thought I could share sooner.
I mentioned this before, but I really like how the currentUser.preferences
proxy works. 👏 Sending requests under the hood, using shallow reactivity, and returning a nice default even when the user doesn't have any preferences for a specific project. 👍
preferences = { | ||
site: {}, | ||
projects: {}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than expecting the full preferences
object here, maybe it would be useful to allow tests to specify a partial object, then merge that partial object with the full object below. So here, it would be:
preferences = {},
And below, it would be:
// mergeDeepLeft() is from ramda
preferences: mergeDeepLeft(preferences, {
site: {},
projects: {}
}),
That would allow a test to easily specify a site preference without also specifying projects: {}
, e.g., something like:
// test/components/project/list.spec.js
it('uses the projectSortMode preference', () => {
// Specifying site without projects.
mockLogin({ preferences: { site: { projectSortMode: 'alphabetical' } } });
createProjects([
{ name: 'A' },
{ name: 'B', lastSubmission: ago({ days: 5 }).toISO() }
]);
const component = mountComponent();
const blocks = mountComponent().findAllComponents(ProjectHomeBlock);
// ['B', 'A'] is the default, as B has the latest data.
blocks.map(block => block.props().project.name).should.eql(['A', 'B']);
const select = component.getComponent(ProjectSort);
select.props().modelValue.should.equal('alphabetical');
});
Just an idea! Could also come later once tests are added.
669a062
to
4bb37a6
Compare
…d privatize some class members
@@ -15,18 +15,21 @@ import { mergeDeepLeft } from 'ramda'; | |||
import configDefaults from '../config'; | |||
import { computeIfExists, hasVerbs, setupOption, transformForm } from './util'; | |||
import { noargs } from '../util/util'; | |||
import { _container } from './resource'; | |||
import UserPreferences from './user-preferences/preferences'; | |||
|
|||
export default ({ i18n }, createResource) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first argument here, from which i18n
is destructured, is the container. You can get http
from there as well.
@@ -15,18 +15,21 @@ import { mergeDeepLeft } from 'ramda'; | |||
import configDefaults from '../config'; | |||
import { computeIfExists, hasVerbs, setupOption, transformForm } from './util'; | |||
import { noargs } from '../util/util'; | |||
import { _container } from './resource'; | |||
import UserPreferences from './user-preferences/preferences'; | |||
|
|||
export default ({ i18n }, createResource) => { | |||
// Resources related to the session | |||
createResource('session'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you assign this resource to a variable, you can reference it below.
createResource('session'); | |
const session = createResource('session'); |
/* eslint-disable no-param-reassign */ | ||
transformResponse: ({ data }) => { | ||
data.verbs = new Set(data.verbs); | ||
data.can = hasVerbs; | ||
const { requestData, http } = self[_container]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above: you should be able to access session
and http
without referencing _container
.
@@ -51,7 +51,7 @@ class BaseResource { | |||
} | |||
} | |||
|
|||
const _container = Symbol('container'); | |||
export const _container = Symbol('container'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_container
is supposed to work like a private property, so it'd be great to avoid exporting it.
return shallowReactive(data); | ||
} | ||
/* eslint-enable no-param-reassign */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to re-enable this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file still need to exist? It looks like the code was moved elsewhere.
Towards getodk/central#689
Supersedes #1021 which superseded #1018
Related: getodk/central-backend#1184 (backend)
This introduces persistent user preferences as per getodk/central#689.
They are exposed on the currentUser resource as
currentUser.preferences
, split up insite
andprojects
objects. These objects are proxied to enable some — hopefully comfortable — abstractions for working with preferences, egcurrentUser.preferences.projects[someProjectID].someproperty = "camembert"
will work even ifprojects[someProjectId]
doesn't actually exist yet (it will be autocreated).The
PUT
andDELETE
HTTP requests necessary for propagating values to the backend are implemented as side effects via these proxy objects, too.The semantics of the preferences values are completely up to the frontend, but the reactiveness enabled is shallow, thus complex preference values (eg objects, arrays) are discouraged — those are inconvenient anyway; more granular key-value pairs are preferred as those will result in less clobbering under concurrent activity from multiple sessions.
Implemented preferences:
Todo:
For later: