Implemented non-RDF fetch #104

NSeydoux · 2020-05-27T08:25:59Z

New feature description

This PR adds a new function exposing a high-level feature to fetch non-RDF data, while using the internal fallback fetch.

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

Some small typo and style suggestions inline, and a couple of questions as food for thought, but I tested this locally and it works flawlessly :)

(That's an image, shown on the page, after having been fetched from my Pod, where it is set to private.)

src/nonRdfData.test.ts

src/nonRdfData.ts

Vinnl · 2020-06-02T10:24:12Z

Ah btw, I just realised that what I didn't test is whether this actually works with solid-auth-fetcher as well. Given that that was explicitly one of Kevin's concerns, probably good to do that :P Did you happen to have given that a try?

NSeydoux · 2020-06-02T10:37:31Z

I did, but for unauthenticated fetches only, because it works directly in command line, which is convenient as opposed to a full-blown webapp where a little more configuration is required. I have been playing around with small webapps yesterday for practice, but they use the @solid/react components which currently have an issue with solid-auth-fetcher. I'll create a plain web app to test this.

NSeydoux · 2020-06-02T15:38:15Z

Good news: with @inrupt/solid-auth-fetcher@0.0.6, it works.

Vinnl

Added one new comment for consideration, and two remarks w.r.t. the new changes. There are also a number of conversations that are marked as resolved, even though I don't see either the suggested changes or a response about why they do not apply?

Vinnl · 2020-06-03T09:02:52Z

src/index.test.ts

@@ -60,6 +61,7 @@ import {
 // These tests aren't too useful in preventing bugs, but they work around this issue:
 // https://github.com/facebook/jest/issues/10032
 it("exports the public API from the entry file", () => {
+  expect(getFile).toBeDefined();


Oh huh, seeing these two next to each other, I'm starting to wonder whether we shouldn't actually call it fetchFile for consistency.

My thinking was that there is going to be a deleteFile and a createFile, and getFile is meant to only support a GET request, so fetchFile may sound a little too generic for people used to the fetch API being a gateway to all HTTP requests. However, given that we use fetch for the 'retrieve' operation everywhere, you are right, this would be more consistant. I'll make the change.

Ah that's a good point - shouldn't there be equivalent methods for LitDatasets? Given the "Planned API" page, it seems that createFile would be saveFile (or saveLitDataset would be createLitDataset - though maybe that doesn't cover that it could also overwrite existing datasets, just like (I presume) createFile could overwrite existing files?), and that we haven't thought of having a deleteLitDataset yet - which I presume we should have? (And which is probably an alias of deleteFile or vice versa.)

Right, I didn't think about the overwriting of existing files. I think saveFile would be good, and we can discuss the exact behaviour in the appropriate PR/ticket.

And adding a deleteLitDataset seems to be something that we should plan. Also, 1b56737 changes getFile into fetchFile

Vinnl · 2020-06-03T09:13:42Z

src/nonRdfData.ts

+  init: Partial<GetFileOptions> = defaultGetFileOptions
+): Promise<Response> {
+  const config = {
+    ...defaultGetFileOptions,
+    ...init,
+  };
+  // The `fetch` field is not part of the original RequestInit, and it is no longer
+  // needed in the init object.
+  delete init.fetch;
+  return config.fetch(input, init);


I think I'd go for separate the options specific to getFile from the options passed to fetch, rather than explicitly deleting non-fetch options, e.g.:

Suggested change

init: Partial<GetFileOptions> = defaultGetFileOptions

): Promise<Response> {

const config = {

...defaultGetFileOptions,

...init,

};

// The `fetch` field is not part of the original RequestInit, and it is no longer

// needed in the init object.

delete init.fetch;

return config.fetch(input, init);

options: Partial<GetFileOptions> = defaultGetFileOptions

): Promise<Response> {

const config = {

...defaultGetFileOptions,

...options,

};

return config.fetch(input, config.init);

Otherwise, every option ever added to getFile will also require adding another delete statement.

That's a good point. Fixed in d1a2fc8

Vinnl · 2020-06-03T09:14:54Z

src/nonRdfData.test.ts

+
+    expect(mockFetch.mock.calls).toEqual([["https://some.url", {}]]);
+  });
+


Maybe we should also have a test that checks that the Response is actually returned to the caller?

Fixed in ee40efb

NSeydoux · 2020-06-03T09:24:33Z

Good catch about the resolutions ! I think one of the commits in the suggestion batch had an issue (it did not introduce changes), and it messed up the whole batch.

Vinnl · 2020-06-03T11:00:56Z

src/nonRdfData.ts

+  init?: RequestInit,
+  options: Partial<GetFileOptions> = defaultGetFileOptions


Haha, sorry to keep going on about this, but given that init is optional, wouldn't it be better as a property on options (i.e. options.init)? Because now if you want to pass a different fetch (or a different option that's added in the future) but not a different init, you'd have to do fetchFile('https://some.url', undefined, { fetch: fetch }).

Suggested change

init?: RequestInit,

options: Partial<GetFileOptions> = defaultGetFileOptions

init?: RequestInit,

options: Partial<GetFileOptions> = defaultGetFileOptions

Don't worry, I was hesitant in the implementation exactly for this reason :). Fixed in c676f1c

src/nonRdfData.test.ts

src/nonRdfData.ts

getFile fetches a data blob, and returns it to the user. It uses the underlying fallback fetch, and supports content negociation by allowing the user to pass in headers.

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>

In order to not require a documentation additional to the fetch API's, the non-RDF fetching will be closer to the original fetch.

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>

NSeydoux requested review from Vinnl and pmcb55 May 27, 2020 08:25

Vinnl approved these changes May 27, 2020

View reviewed changes

NSeydoux force-pushed the feat/74-read-non-rdf-data branch 2 times, most recently from 16d38b8 to eb29a5f Compare June 2, 2020 08:32

NSeydoux requested review from Vinnl and ajacksified June 2, 2020 16:25

Vinnl approved these changes Jun 3, 2020

View reviewed changes

NSeydoux force-pushed the feat/74-read-non-rdf-data branch from d1a2fc8 to 3bfa072 Compare June 3, 2020 10:50

Vinnl reviewed Jun 3, 2020

View reviewed changes

src/nonRdfData.test.ts Outdated Show resolved Hide resolved

Vinnl approved these changes Jun 3, 2020

View reviewed changes

src/nonRdfData.ts Outdated Show resolved Hide resolved

ajacksified approved these changes Jun 3, 2020

View reviewed changes

NSeydoux and others added 11 commits June 3, 2020 17:34

Implemented non-RDF fetch

f1db923

getFile fetches a data blob, and returns it to the user. It uses the underlying fallback fetch, and supports content negociation by allowing the user to pass in headers.

Apply suggestions from code review

197f5fc

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>

Applied suggestions from PR#104

7ccd667

Brought the non-rdf fetching closer to the fetch API

b518af6

In order to not require a documentation additional to the fetch API's, the non-RDF fetching will be closer to the original fetch.

Implemented PR feedback

09e865c

Renamed getFile into fetchFile

209fd74

Checks the returned response in tests

3d0a492

Split between the RequestInit options and the the fetcher options

8195e0e

Fixed the way options are passed to fetchFile

489b059

Removed outdated comment

b811e17

Update src/nonRdfData.ts

41b82c0

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>

NSeydoux force-pushed the feat/74-read-non-rdf-data branch from 89889a2 to 41b82c0 Compare June 3, 2020 15:34

NSeydoux merged commit 25deaa9 into master Jun 3, 2020

NSeydoux deleted the feat/74-read-non-rdf-data branch June 3, 2020 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented non-RDF fetch #104

Implemented non-RDF fetch #104

NSeydoux commented May 27, 2020

Vinnl left a comment •

edited

Vinnl commented Jun 2, 2020

NSeydoux commented Jun 2, 2020

NSeydoux commented Jun 2, 2020

Vinnl left a comment

Vinnl Jun 3, 2020

NSeydoux Jun 3, 2020

Vinnl Jun 3, 2020

NSeydoux Jun 3, 2020

NSeydoux Jun 3, 2020 •

edited

Vinnl Jun 3, 2020

NSeydoux Jun 3, 2020

Vinnl Jun 3, 2020

NSeydoux Jun 3, 2020

NSeydoux commented Jun 3, 2020

Vinnl Jun 3, 2020

NSeydoux Jun 3, 2020


		expect(mockFetch.mock.calls).toEqual([["https://some.url", {}]]);
		});

		init?: RequestInit,
		options: Partial<GetFileOptions> = defaultGetFileOptions

Implemented non-RDF fetch #104

Implemented non-RDF fetch #104

Conversation

NSeydoux commented May 27, 2020

New feature description

Checklist

Vinnl left a comment • edited

Choose a reason for hiding this comment

Vinnl commented Jun 2, 2020

NSeydoux commented Jun 2, 2020

NSeydoux commented Jun 2, 2020

Vinnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NSeydoux Jun 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NSeydoux commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl left a comment •

edited

NSeydoux Jun 3, 2020 •

edited