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

Added Solid support #1319

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Conversation

yasharpm
Copy link

Hello, I have implemented the Solid backend. These changes are compatible with the widget. I am creating another pull request for the widget as well.

The test code and documentation updates are also there...

Thank you :-)

@yasharpm
Copy link
Author

@raucao
Copy link
Member

raucao commented Aug 29, 2024

Great to see a PR for another open protocol!

Feel free to also notify people on the community forums about these PRs: https://community.remotestorage.io/t/adding-solid-as-a-backend/828

Maybe you could also add some information for people new to SOLID (like myself e.g.) about how to test this end-to-end (i.e. which server implementation and.or popular provider/hoster to use).

Copy link
Contributor

@DougReeder DougReeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wonderful; thank you!

A big change like this will take us a while to review and may take a couple rounds of changes (hopefully small); please be patient.

1. Have you had a chance to run the API test suite against this? rs-backup tweaked for your server is the easiest way to generate a token, that I've found.

  1. Do you know if the Solid library is using fetch or XHR? We've been supporting environments with either one; if Solid only uses fetch we'll need to document that.

  2. I see you've written tests using Jaribu. Unfortunately, that has been deprecated and doesn't run under recent versions of Node.js. New tests are being written using Mocha. I know it's a pain, but could you please add Mocha tests for Solid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add some comment lines, explaining how this file fits into the architecture?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this interface is only used for Solid code, then you can move it to the Solid module and export it from there.

@@ -368,7 +370,7 @@ export class RemoteStorage {
if (typeof cfg === 'object') { extend(config, cfg); }

this.addEvents([
'ready', 'authing', 'connecting', 'connected', 'disconnected',
'ready', 'authing', 'connecting', 'pod-not-selected', 'connected', 'disconnected',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine some reasons for 'pod-not-selected' to be separate from 'error', but please detail them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... because no existing app will respond to the 'pod-not-selected' event, but they will respond to other events.

Copy link
Member

@raucao raucao Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the state of the Solid connection process for the widget or someone implementing custom UI. So it should be part of a documentation page for adding Solid to an app (maybe another section on the GDrive/Dropbox page, and renaming the page and menu item).

@raucao
Copy link
Member

raucao commented Aug 29, 2024

  1. Have you had a chance to run the API test suite against this?

The API test suite is irrelevant here, as it tests the compliance of RS servers with the RS specification. However, this PR is for clients connecting to Solid servers, as an alternative to RS servers.

@raucao
Copy link
Member

raucao commented Aug 29, 2024

Another note for tests: there is a new Mocha suite for the RemoteStorage class in this open PR: #1318

@DougReeder
Copy link
Contributor

  1. Have you had a chance to run the API test suite against this?

The API test suite is irrelevant here, as it tests the compliance of RS servers with the RS specification. However, this PR is for clients connecting to Solid servers, as an alternative to RS servers.

Right you are! I've been wrestling with Armadietto for half a year. :-S

@yasharpm
Copy link
Author

yasharpm commented Aug 30, 2024

Maybe I need to add a full documentation on how to directly incorporate the Solid backend without using the widget?

When testing with a real app, does calling setAuthURL() eliminate the need for the updated Widget? And what object has that method?

It does. The object that has the method is Solid. Accessible via remoteStorage.solid.setAuthURL(). But we would still have the pod-not-selected step which needs to be called if the developer is not using the widget.

dropbox-and-googledrive.md is somewhat vague.

I'll fix it. I believe I renamed the file to dropbox-googledrive-solid.md.

More generally, when I pull this version of remotestoragejs into a real app, what do I need to do to connect to a pod? (I have a pod on Inrupt)

const connectTask = setTimeout(() => {
  remoteStorage.solid.setAuthURL('your-solid-identity-provider'); // i.e. https://login.inrupt.com
  remoteStorage.solid.connect();
  // Calling 'connect()' will immediately redirect to your identity provider website.
}, 1000);
remoteStorage.on('pod-not-selected', () => {
  clearTimeout(connectTask);
  const podURLs = remoteStorage.solid.getPodURLs();
  // Choose one. Maybe there is even 0?
  remoteStorage.solid.setPodURL(podURLs[0]);
  // That's it. 'connected' is fired immediately.
};
remoteStorage.on('connected', () => {
  // We are connected.
  clearTimeout(connectTask);
  // We arrive here either through calling 'setPodURL' on the `pod-not-selected` event or
  // on page getting loaded. Because: Inrupt Solid library prefers to always redirect to the
  // identity provider to get the session tokens. I spent a lot of time on this. If we want to
  // change it, we'll have to make fundamental changes to Inrupt's library and make a PR.
});

Calling connect() redirects the page. And we get back to our homepage with a payload containing the session data. So whenever our home page is loaded, we are either not connected at all or in the process i.e. we get a session but have not selected the pod yet, or a pod was already selected and we immediately jump to the connected state.

@DougReeder
Copy link
Contributor

My line of logic is that, if the developer is using the widget, they don't need to worry about anything. The Solid option does not even show up if they don't put it in the widget's configuration. They need to worry about it only if they are bypassing the widget and intent to add support for Solid. It doesn't affect the existing apps.

Great! Please add a comment in the code and update the event documentation, to that effect.

@DougReeder
Copy link
Contributor

If you have an app that uses this, it would help if we could look at it.

@DougReeder
Copy link
Contributor

I am almost certain that it only uses fetch. I can do a through inspection and report back if necessary.

I believe the vast majority of browsers in use support fetch, but the API didn't fully stabilize in Node.js until v21. Security support for v20 ends on 30 Apr 2026 (in 1 year and 8 months) If I understand correctly, the implementation in v18 and v20 doesn't support streams fully.

So, we'll need to fully test this under Node 18 and 20, to see if this works properly with them. If it doesn't, I'm okay with documenting that this version of remotestorage.js when used in Node.js, should only be used with v21 and later.

@raucao
Copy link
Member

raucao commented Aug 30, 2024

According to our docs, fetch is available from node.js 18 onwards, and we already ask to add a polyfill for when it's not. So the usage of fetch in a new back-end shouldn't change anything for node.js users or in our docs.

https://remotestorage.io/rs.js/docs/nodejs.html#caveats

@yasharpm
Copy link
Author

yasharpm commented Sep 2, 2024

I've updated the documentation and added mocha tests. This should be it as far as I followed the conversation. What do you guys think?

@DougReeder
Copy link
Contributor

I haven't been able to use this in a real app yet — the app I've been testing with is throwing up issues that don't appear to be caused by this. I'll have to try with a different app.

@NoelDeMartin
Copy link

Hi there, I don't know much about remoteStorage; but I've been working on Solid Apps and something crucial to Solid is using RDF for interoperability. Seeing this PR, I understand that this adds support for a Solid backend as a "file storage", right?

I wonder how would anyone use this library to make a Solid application using RDF. Would the developers need to write RDF manually? What about updates to documents?

Anyways, I'm just mentioning this in case you weren't aware. If that's already the intention, using Solid as a file storage rather than a store of RDF documents, that's fine. But I think using a Solid POD like that is missing a lot of the potential. I guess it's better than nothing though, if it adds "Solid support" to some of the existing apps implemented using remoteStorage. But I don't think I'd recommend anyone making a Solid App from scratch to follow this approach.

By the way, in case it's useful for anyone, some years ago we had a discussion comparing remoteStorage, Fission, and Solid. And this was one of the points that came up: https://vimeo.com/779640162

@DougReeder
Copy link
Contributor

Anyways, I'm just mentioning this in case you weren't aware. If that's already the intention, using Solid as a file storage rather than a store of RDF documents, that's fine. But I think using a Solid POD like that is missing a lot of the potential. I guess it's better than nothing though, if it adds "Solid support" to some of the existing apps implemented using remoteStorage. But I don't think I'd recommend anyone making a Solid App from scratch to follow this approach.

Yes, you won't get many of the benefits of a native Solid app. But this is an important step in moving people to storage independent of apps. With this, someone who has a Solid pod can also use the same storage for Apps written for remoteStorage.

@raucao
Copy link
Member

raucao commented Sep 12, 2024

Wouldn't that be something that could be supported in the data modules for this library?

@yasharpm
Copy link
Author

Wouldn't that be something that could be supported in the data modules for this library?

This was the idea when we were considering the task. The data module receives the Client instance which probably exposes the RemoteStorage hence the Solid backend which then you can get the raw Solid Session from it.

So a data module can implement a specific behavior for dealing with Solid the way it is recommended. The downside is that in order to be Solid friendly is that the different data modules need to have a check for the back-end type and do different thins.

@DougReeder
Copy link
Contributor

I've tried adding this and the updated widget to a simpler app, but no option for Solid storage appears in the list. Is there an example of this running in a real app?

michielbdejong added a commit to remotestorage/myfavoritedrinks that referenced this pull request Dec 11, 2024
@michielbdejong
Copy link
Member

I'm trying this out in remotestorage/myfavoritedrinks#15

@michielbdejong
Copy link
Member

There seems to be a bug related to folder listing. Storing 'beer' as my favorite drink works, but then it repeats GET 'https://michielbdejong.solidcommunity.net/myfavoritedrinks/' -H 'accept: text/turtle' in a loop and 'synchronizing' spins indefinitely. This is the response body. Is it supposed to fetch Turtle, or was the intention to fetch the folder listing as JSON-LD?

@prefix : <#>.
@prefix dct: <http://purl.org/dc/terms/>.
@prefix ldp: <http://www.w3.org/ns/ldp#>.
@prefix stat: <http://www.w3.org/ns/posix/stat#>.
@prefix xsd: <http://www.w3.org/2001/XMLSchema#>.
@prefix myf: <>.
@prefix json: <http://www.w3.org/ns/iana/media-types/application/json#>.

myf:
    a ldp:BasicContainer, ldp:Container;
    dct:modified "2024-12-11T13:42:10Z"^^xsd:dateTime;
    ldp:contains myf:beer;
    stat:mtime 1733924530.183;
    stat:size 4096 .
myf:beer
    a json:Resource, ldp:Resource;
    dct:modified "2024-12-11T13:42:10Z"^^xsd:dateTime;
    stat:mtime 1733924530.183;
    stat:size 88 .

@michielbdejong
Copy link
Member

I found a bug in this PR. The code that produces folder item maps is not including ETags (lines 414 and 419 of src/solid.ts)
But sync considers an items map without ETags as corrupt. If the server doesn't give ETag headers, I think we can just use the last-modified dates as etags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants