pos-switch with reactivity and testing presence of forward and backward links#193
pos-switch with reactivity and testing presence of forward and backward links#193
Conversation
776e290 to
b5e8a10
Compare
75dcbeb to
6894935
Compare
|
@angelo-v - thanks for the review. Let me know if you consider the issues to be resolved - hopefully the tests will now be easier to assess. Note that additional fixes were made to |
56965d7 to
0a86a49
Compare
| return merge(this.store.additions$, this.store.removals$).pipe( | ||
| // Note: we assume that cost of filtering by the optional predicate is not worthwhile | ||
| filter((quad) => quad.subject.value == this.uri), | ||
| debounceTime(250), |
There was a problem hiding this comment.
question: why do we use debounce here but not on observeTypes?
There was a problem hiding this comment.
My reasoning is still developing, so would be good to discuss and see if there's a better solution:
- I consider debounceTime to be a potentially buggy heuristic. What we want to know is that there will not be in any more emissions that would cause a rerender. I've set the time rather conservatively at 250ms. This is probably too long because it's latency that a user might notice, but on a slow device the user impact of rerendering could be worse. I expect we'll want to reduce the time but would want some evidence to support the selection of a new value.
- So my preference was to avoid debounceTime if an argument can be made to do so. Relations are the work horse of RDF, so we expect them to change frequently and for groups to be updated at once when documents are loaded. They're also used frequently across the user interface so potentially have a big effect on rerendering (my early experiments had some very painful churn). Types are updated infrequently and are used less frequently in the user interface. I expect the first set of types to be already loaded in the store when first called, so emissions are likely to be one at a time when changes are made. It therefore felt safe to avoid the debounceTime.
I think eventually we'll need e2e tests with performance metrics and that'll provide some evidence to improve this, but in the mean time any anecdotal feedback would be good too.
We can maybe open an issue related to rerendering performance?
There was a problem hiding this comment.
Thanks, sounds reasonable for now, but I am in favor of opening an issue to document that beyond this comment.
|
I did a rebase and some minor improvements. Looks good for me. Despite the question about the debounce I will merge this, if it turns out we will need to add a debounce there we can do it on main or a new branch |
Closes #183
Definition of Done