-
Notifications
You must be signed in to change notification settings - Fork 3
List things of a specific type, with a custom display #140
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
base: main
Are you sure you want to change the base?
Conversation
887a39e
to
e64f73d
Compare
<pos-list all-from-store if-typeof=${type} ?fetch=${fetch}> | ||
<template> :-) </template> | ||
</pos-list> | ||
`} |
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've run into a problem with the design of this functionality.
By design, I want to list everything in the store, but because we haven't tackled reactivity yet (#37), all the resources to be shown need to have been loaded before pos-list
is rendered.
In my own experiments, this hasn't been a problem because 1) I have been using a rudimentary reactive store and 2) before then, I was using a component that loaded a resource and then triggered a re-render of HTML elements matching a CSS selector - which provided crude reactivity to at least that resource being loaded.
Until we implement #37, pos-list
with if-typeof
will still be usable in cases where its rendering is delayed by other components, but I can't think of an easy way of demonstrating this in this story.
Even putting it in pos-resource
(which is misleading because it does not receive or use the resource) does not work because there seems to be a race condition in which pos-list
receives the os
object and calls findMembers
before pos-resource
has finished processing???
I think we have at least three options:
- Implement reactivity for
findMembers
, as a PoC for Reactivity to state changes #37 - Implement a
doc
attribute -if-typeof
would then be scoped to that document, and we can test the document is loaded before rendering. Listing from the whole store would come later. - Park this PR until after Reactivity to state changes #37
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.
As I understand it the race-condition comes from pos-list being in the light-dom and triggering componentWillLoad even if the slot is not renderd by pos-resource, just because it is part of the dom already as a child of pos-resource. This issue would also occur if authors of html dashboards used it that way. There would be no notion to say "wait for this resource to load first". So the pragmatic approach would be to add the doc attribute. But we need to take care of reactivity soon, since it is quite fundamental.
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 I have a fairly elegant solution for Option 1 using rxjs to process the event feed generated by addDataCallback rdflib.js
The interface will end up something like:
os.store.observeFindMembers(
this.ifTypeof,
this.disconnected$
).subscribe(value => this.items = value)
I'll add it to this PR and we can confirm whether it's viable.
I like the idea of the components reacting to an event stream rather than to a change in state.
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.
Looks like the storybook now works.
Hopefully I covered everything with unit tests. I did try to follow TDD.
If this looks ok as a solution to you, I'll add an integration test.
There's a number of further improvements I could add to this PR already if you think it's worthwhile
- ADR for this reactivity approach
- Handling of of removal of quads from the store. Either the stream$ needs to have an extra indication of whether the quad is added or removed, or we could have two separate streams e.g. additions$ and removals$. Either way, these are then used by other core functions rather than directly by elements.
- Handling of rdfs:subClassOf in observeFindMembers - currently adding a quad that establishes a new subclass wouldn't trigger an update.
- Handle changes in the members not just changes in length of the members. This is necessary if removal of quads is supported, but is a potential performance issue - every new quad matching the filtered predicates will trigger the distinctUntilChanged comparator function.
- In principle, I would want observeFindMembers to also use debounceTime - if a lot of quads come in within 100ms, I would want a single emission to be made. However, that will add at minimum a 100ms delay to all tests. I'm not sure what is the best way of handling this.
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'm going ahead with these reactivity features:
- Handling of additions of quads with predicate
rdf:type
- Handling of of removal of quads from the store
- Handling of
rdfs:subClassOf
inobserveFindMembers
Handle changes in the members not just changes in length of the members. This is necessary if removal of quads is supported, but is a potential performance issue - every new quad matching the filtered predicates will trigger thedistinctUntilChanged
comparator function.
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.
regarding the debounce: this would be a good idea imho. You can use it using fake timers, as I did with the debounce in the nav bar, compare https://github.com/pod-os/PodOS/blob/main/elements/src/components/pos-navigation/__test/typeToSearch.ts#L9
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.
ADR would be definetly be necessary, but you can do it seperately after we merged. DoD is not required to merge a PR, but to consider a new feature "done"
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'd be happy for you to revert or cherry pick commits if you'd prefer not to merge the removals reactivity yet. I ended up implementing it because I needed to be sure it would work. Good thing I did because there were more challenges than expected.
I don't know why rdflib.js does it, but rdfArrayRemove
is indeed exposed as an option as the official way of overriding quad removal.
I'm not patching anything - I'm using the public options argument!
It's probably a historical anomaly.
I'll add a separate comment explaining the observableGraph function. I had a post-rdflib.js world in mind when I wrote it.
I had a think about going the doc route and realised that it wasn't necessarily going to be easier because pos-list doesn't do its own fetching. I would either need to observe doc changes or implement doc fetching somehow.
So filtering doc, ADR and debounce were all descoped from this PR to make it slightly less monstrous.
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.
We could ask for clarification in rdflib or solidos chat perhaps somebody knows more and can propose a way forward
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.
With rdflib 2.3.0 you can now use the data removal callback
a0b3414
to
c0ab0e1
Compare
1407e33
to
b7d07a0
Compare
…Of is encountered
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.
some remarks following convential comments pattern https://conventionalcomments.org
return Object.keys(this.internalStore.findMemberURIs(sym(classUri))); | ||
} | ||
|
||
observeFindMembers( |
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.
todo: Please add a typedoc comment here as well
<pos-list all-from-store if-typeof=${type} ?fetch=${fetch}> | ||
<template> :-) </template> | ||
</pos-list> | ||
`} |
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.
Wow, addDataCallback
really is cool, I did not know it existed. All the more it is a shame, that it only works for addtions and not removals. I am not so happy with PodOS patching it in, I think this should be something that we add upstream, directly to rdflib.js? Or am I misunderstanding something?
I just made it possible to pass in a custom rdflib store in the PodOS constructor, but this would not be possible anymore if PodOS relies on a new observable store. Or at least you won't get the reactivity if you do this, which can also be confusing.
I am a bit worried that this PR get's out of hand. We should definetly follow up on the reactivity approach, but I wonder if we can get to a mergable solution for pos-list without the observable store? Perhaps only support reactivity for additions (this would not need the custom store class then, correct?) Or we go with the doc
attribute first.
it('children render label for all things of the given type, reactively', async () => { | ||
const os = mockPodOS(); | ||
const observed$ = new Subject<String[]>(); | ||
const firstArg = matcher => when.allArgs((args, equals) => equals(args[0], matcher)); |
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.
This can probably simplified if we get rid of the stop$ param
statements[i].object.equals(quad.object) && | ||
statements[i].graph.equals(quad.graph) | ||
) { | ||
//console.log(quad) |
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.
todo: remove comment
|
||
componentWillLoad() { | ||
subscribeResource(this); | ||
if (this.rel) subscribeResource(this); |
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.
issue: There is an issue with both rel and if-type-of: The resource does not reflect changes to these attributes. The data has to be updated on each change of those. It's fine for me to approach this issue in a separate MR.
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.
My intention was that those attributes should not be mutable - they are typically hard coded when writing HTML. But yes, we can add an issue to add this feature.
|
||
receivePodOs = async (os: PodOS) => { | ||
this.os = os; | ||
os.store.observeFindMembers(this.ifTypeof, this.disconnected$).subscribe(value => (this.items = value)); |
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.
issue: use takeUntil(disconnected$)
here or store the subscription and unsubscribe in disconnectedCallback()
instead of passing disconnected$
|
||
observeFindMembers( | ||
classUri: string, | ||
stop$: Subject<void>, |
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.
issue: dont pass stop$
as a param. The consumer will get a subscription when calling subscribe()
and can unsubscribe itself
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.
Let's maybe find a time to discuss.
My intention was that observeFindMembers should not be performing all these filtering computations if there's no listener.
Maybe I misunderstood rxjs - does this pipe stop automatically if the consumer unsubscribes?
Using an RDF store as a global store means that reactivity can be very expensive - we have to listen to quad additions and removals rather than e.g. being able to proxy variable changes on an object. It's therefore important that the filtering only happens when it's needed.
Other optimisations might also be possible so that some of these filtering steps are reusable across different observe functions. I think that's something we could return to.
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.
Nothing happens if there is no subscriber. And if you call observeFindMembers twice, you will have two subscriptions, both having their own pipe
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 if you want to share parts of the pipe you have to explicitly use share()
. There is also a difference between cold and hot observables (https://luukgruijs.medium.com/understanding-hot-vs-cold-observables-62d04cf92e03) I am not an expert on this and would have to re-read all this myself, but I hope that helps a bit already
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.
Unsubscribe + share sounds like what I am after.
Presumably the resulting observable also needs to be cached and reused rather than recreated. That should be easy here using classUri as a key.
If I understand correctly, I don't need to worry about destroying these cached observables given that nothing happens if there is no subscriber.
private readonly internalStore: | ||
| IndexedFormula | ||
| ObservableIndexedFormula = observableGraph(), | ||
) { |
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.
My approach here was to fully encapsulate reactivity with a minimal and optional API change - the only changes to the IndexedFormula is the new observables, and the app using PodOS can implement those observables any way it wants - the ability to override the store is maintained.
What I haven't maintained is the ability to pass an IndexedFormula and get reactivity for free. This is for two reasons: 1) implementation of the observables is a low level graph store feature that should be independent of PodOS - if I switch away from rdflib.js I should be able to implement the observables any way I want, 2) the alternative is to impose rdflib.js' public API for adding observability. If I hadn't run into problems with removals I would have done that - IndexedFormula would be required to have an addDataCallback
method. Because I was forced to move away from that, I've settled on a solution that I actually prefer - I don't think addDataCallback
should be the public API that PodOS expects?
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.
addDataCallback
is already in the public API of IndexedFormula. The problem left over is really just the removals. I wonder why the data callback only handles addtions and not removals. Perhaps this is something that should change in rdflib?
What I don't like with the current solution is that pos-list will not work if you pass your own IndexedFormula to PodOS. This goes against the principle of least surprise, because there is no indication to the user that this functionality is dependent on not passing a custom store.
Imho the ideal API would be:
- user passes an indexed formula
- if not we create one
- we call addDataCallback internally to add the reactivity feature
- this handles deletions as well as insertions
Co-authored-by: jg10 <[email protected]>
Closes #104
Implements
<pos-list if-typeof="http://schema.org/Recipe">
Definition of Done
Keep a Changelog