First draft of DiscoveryClient and SubscriptionClient

[elf-pavlik]

TODO

• Test

[elf-pavlik]

I tried using LDO first but for shapes that define objects as IRI (very common in Solid Notifications Data Model), it was providing objects with service.channelType['@id'] which felt bit counter productive.

[jaxoncreed]

Hey I'm sorry for my tardiness. This looks like a big refactor, so I'll bring it down and do some testing once everything is done.

I tried using LDO first but for shapes that define objects as IRI (very common in Solid Notifications Data Model), it was providing objects with service.channelType['@id'] which felt bit counter productive.

It's a little counter-intuitive, but it's by design. The philosophy is that LDO should handle objects consistently. An ID needs to be accessed via the "@id" field, and an IRI object can't just be a string. It's to handle this usecase:

The developer has a profile with a list of friends. The shape says that each friend is an IRI, but the developer also wants to load all the friends into the same dataset (so we've brought in data from multiple resources now). If LDO returned a string for the IRI type, the developer would not be able to see the additional data that was loaded from the friend's profile.

I know that use case doesn't apply to this scenario, but needing to use the "@id" field for IRI objects is a small price to pay for consistency.

[elf-pavlik]

This looks like a big refactor, so I'll bring it down and do some testing once everything is done.

Besides, really great setup for testing, which I didn't change, there was only incomplete implementation of one function:
https://github.com/o-development/solid-notification-client/pull/1/files#diff-89ea7052b2aafdd8ad02eec3346c65367e7b06b572d7ddad708291838d82c5df

You can run it locally and all the tests should pass. It would be great to hear your thoughts on the approach I took here.
I don't think there is a need to have functions with always require the caller to pass the authenticated fetch. Creating an instance of a client and passing authenticated fetch in the constructor seems more ergonomic to me. All methods for the clients only require passing the topic and the channel type. At this stage, it doesn't support any notifications features since all features are optional and we can always add once we have all the required functionality.

[jaxoncreed]

Sounds good. I'll clone this down and look at it, though I'll warn that I have quite a bit on my priority stack, so it might not be fore a few days.

[elf-pavlik]

Thanks! I will also try to get in touch with @woutslabbinck who developed https://github.com/woutslabbinck/Solid-Notification maybe we could find a way to collaborate on one implementation which meets everyone's requirements. I would be happy if that worked out.

[elf-pavlik]

I'm already starting to use this branch in sai-js
Since until we cut the 1.0 release we can make breaking changes, do you see a problem with getting the current state of this PR released as 0.1 and addressing the TODO list as separate issues releasing them as 0.2, 0.3, etc?

[woutslabbinck]

I've installed it and tested it with following code and a CSS v6.0.1 with WebSocketChannel2023 enabled:

const {DiscoveryClient} = require('@solid-notifications/discovery');
const {SubscriptionClient} = require('@solid-notifications/subscription');

async function main() {
    // test client
    const client = new DiscoveryClient(fetch)
    const websocketURL = await client.findService("http://localhost:3000/resource.ttl","http://www.w3.org/ns/solid/notifications#WebSocketChannel2023")

    console.log(websocketURL);
    // test subscribing
    const subscriptionClient = new SubscriptionClient(fetch)
    const channel = await subscriptionClient.subscribe("http://localhost:3000/resource.ttl","http://www.w3.org/ns/solid/notifications#WebSocketChannel2023")

    console.log(channel);

}

main()

I haven't looked specifically into the code itself, but as of right now it works as how I expected it to work.

One issue that I've had however is that while installing from a fresh clone, I've received the following error:

error Couldn't find package "@solid-notifications/discovery@^0.1.0" required by "@solid-notifications/subscription@0.0.0" on the "npm" registry.

Which I fixed by editing the package.json of discovery: "version": "0.0.0" to "version": "0.1.0".

If you want to release it, you might want to change the subscription package.json version also to 0.1.0 for consistency.

[elf-pavlik]

Thanks for the review @woutslabbinck

If you want to release it, you might want to change the subscription package.json version also to 0.1.0 for consistency.

I reverted all the versions to 0.0.0, this way we should be able to use npx lerna version to bump all the versions.

We probably can cut the version and release it as a separate step after merging this PR.
I'm also going to add dedicated PR with GitHub workflows to run tests and auto-publish to npm on release.

[woutslabbinck]

Looks good to me.
When merged as version 0.0.0 into this repo, I'll start a branch to add some documentation to the most important methods as well (just like I did in solid-notification-client).

I think that'll help others when trying to experiment with Solid Notifications.

[elf-pavlik]

I created a couple of issues which we can resolve in dedicated PRs https://github.com/o-development/solid-notification-client/issues

IMO once we address #2 and #3 we should be able to release it as 0.1 and continue iterating from there.

@jaxoncreed could you please review this PR and possibly merge it?

[jaxoncreed]

Hey sorry for the delay LGTM.