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

Add isLitDataset() function #214

Merged
merged 3 commits into from Jul 2, 2020
Merged

Add isLitDataset() function #214

merged 3 commits into from Jul 2, 2020

Conversation

Vinnl
Copy link
Contributor

@Vinnl Vinnl commented Jun 30, 2020

New feature description

This will allow applications that explore a Pod without knowing
what type of data they might encounter to determine what fetch
method to use when they have WithResourceInfo.

This method (send a HEAD/GET request without an Accept header, check if the Response type includes the spec-mandated Turtle or JSON-LD) of determining whether a Resource is an RDF serialisation understood by the server was recommended by the specification team: https://gitter.im/solid/specification?at=5ee9067da85de30394194d06

Checklist

  • All acceptance criteria are met.
  • Relevant documentation, if any, has been written/updated.
  • The changelog has been updated, if applicable.
  • New functions/types have been exported in index.ts, if applicable.
  • Commits in this PR are minimal and have descriptive commit messages.

@Vinnl Vinnl self-assigned this Jun 30, 2020
@Vinnl Vinnl force-pushed the feat/163-is-litdataset branch 2 times, most recently from c466223 to e8f380a Compare June 30, 2020 14:28
@Vinnl Vinnl mentioned this pull request Jun 30, 2020
5 tasks
@Vinnl Vinnl added the enhancement New feature request label Jun 30, 2020
response.headers.get("Content-Type")?.split(";") ?? [];
const isLitDataset =
contentTypeParts.length > 0 &&
["text/turtle", "application/json+ld"].includes(contentTypeParts[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't application/rdf+xml and RDFa (I don't recall the mime type and I'm on my phone) spec-compliant serializations ?

Copy link

Choose a reason for hiding this comment

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

application/json+ld => application/ld+json

Copy link

Choose a reason for hiding this comment

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

@NSeydoux RDFa typically uses just text/html, so it's hard to identify it as an RDF serialization per se. Plus, there are other complications. application/rdf+xml is legit, but I'd recommend against it -- it doesn't support datasets and ... it's XML. Really, though, if you don't have to support RDF+XML, I'd recommend against it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I would never encourage anyone to use RDF/XML, it's an awful serialization, but if we fetch a resource from a Pod and it happens to be RDF/XML, we should be able to recognize it as an RDF Resource I guess (maybe display a popup that goes "RDF/XML ? REALLY ?"), and if it is allowed by the spec I don't think there's a way around it.

By the way, I just checked, N3 doesn't support RDF/XML. The current spec being ultra-precise (Linked Data resources (RDF in the form of JSON-LD, Turtle, HTML+RDFa, etc), I suggest we leave RDF/XML alone in the cold and not support it. If detecting RDFa is also not possible based on Content-Type, this conversation is resolved.

Copy link

Choose a reason for hiding this comment

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

Suggested change
["text/turtle", "application/json+ld"].includes(contentTypeParts[0]);
["text/turtle", "application/ld+json"].includes(contentTypeParts[0]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think Michiel said that to me once, but I can't really find it. What I can find is this:

IMPORTANT: a default Content-Type: text/turtle will be used for requests for RDF resources or views (containers) that do not have an Accept header.

So maybe I should just remove the JSON-LD MIME type and just check that it's text/turtle?

Copy link

Choose a reason for hiding this comment

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

@Vinnl I wouldn't rely on any statement from the old solid-spec repo. The default of text/turtle is true if the backend is a conforming LDP server, but that isn't a strict requirement of the new Solid spec, so I don't think you can rely on that. Personally, I would leave JSON-LD.

Copy link
Contributor Author

@Vinnl Vinnl Jul 1, 2020

Choose a reason for hiding this comment

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

Thanks @acoburn. But then the follow-up question is: is checking for just Turtle and JSON-LD enough, or is it also allowed for Solid servers to suddenly default to a different representation?

Copy link

Choose a reason for hiding this comment

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

I really don't think you'll find a default representation that's not Turtle or JSON-LD. At least not by something that claims to be Solid compliant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! Thanks :)

@vercel
Copy link

vercel bot commented Jul 1, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/inrupt/lit-pod/bt4shprvz
✅ Preview: https://lit-pod-git-feat-163-is-litdataset.inrupt.vercel.app

This will allow applications that explore a Pod without knowing
what type of data they might encounter to determine what fetch
method to use when they have WithResourceInfo.
@vercel vercel bot temporarily deployed to Preview July 2, 2020 07:35 Inactive
@Vinnl Vinnl merged commit bc178ae into master Jul 2, 2020
@Vinnl Vinnl deleted the feat/163-is-litdataset branch July 2, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants