-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fetch well known solid from root #1604
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 37c102e:
|
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.
Quick initial review, two questions/concerns
); | ||
// Fetches Pod root well known | ||
mockedFetcher.fetch.mockResolvedValueOnce( | ||
new Response( |
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.
Could we store these responses in variables ahead of time? That or in a fixtures directory?
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 would rather be able to read the tests easily ^^,
).href; | ||
return await getSolidDataset(wellKnownSolidUrl); | ||
} catch (e) { | ||
// In case of error, do nothing and try to discover the .well-known |
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.
Would it be worth storing this error in case the rootResource isn't found?
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 fixed up the commnent of the outer function.
All we know here is the server's root well known solid is not accessible, then we can try the other mechanism, when we're done with it, all we know is neither worked... not which one should have worked... I guess the resource might even not be hosted on a solid server ^^,
So, no, I don't think we need to store the error.
For what it's worth, the behaviour was the same prior (the error was catched and the other mechanism tried out).
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 agree with @matthieubosquet: the error that matters is that none of the .well-known
discovery mechanism worked as a whole, rather than detailing each failed attempt. This whole thing is implementation-specific anyway, once the discovery mechanism is set at the spec level this will be completely rewritten.
src/resource/solidDataset.ts
Outdated
@@ -1202,6 +1202,19 @@ export async function getWellKnownSolid( | |||
> = internal_defaultFetchOptions | |||
): Promise<SolidDataset & WithServerResourceInfo> { | |||
const urlString = internal_toIriString(url); | |||
|
|||
// Try to fetch the well-known solid dataset from the server's root (ESS 2.0 behaviour) |
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 is a comment out of scope for the current PR, but do you think it would be worth documenting systematically, for each function of the public API, if it is a function built against the Solid spec or a particular implementation ? Something like a custom TSDoc tag for instance.
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.
Custom TSDoc tag sounds interesting, but I'm not sure it would be very appropriate inside of functions or how standard it is (a quick search yielded an issue, but nothing on the docs).
I see however that there is an experimental tag that I would recommend using for everything that is not standard. I also think it would be good to tag as alpha everything that is not in a versionned standard (at this point in time everything but Solid Protocol).
Furthermore, the see tag seems appropriate for spec links and the label tag is probably the best fit for implementation specific functions.
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 removed this version pointer for now)
).href; | ||
return await getSolidDataset(wellKnownSolidUrl); | ||
} catch (e) { | ||
// In case of error, do nothing and try to discover the .well-known |
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 agree with @matthieubosquet: the error that matters is that none of the .well-known
discovery mechanism worked as a whole, rather than detailing each failed attempt. This whole thing is implementation-specific anyway, once the discovery mechanism is set at the spec level this will be completely rewritten.
This makes fetching the well known solid from the server's root by default.