feat: require content-type parser to set content-type #423

SgtPooki · 2024-02-07T19:53:35Z

Title

feat: require content-type parser to set content-type

Description

adds contentTypeParser function to createVerifiedFetch options & implements it.
renamed getStreamAndContentType to getStreamFromAsyncIterable that now returns a stream with the firstChunk seen, so we can pass it to the contentTypeParser function.
updates tests in packages/verified-fetch & packages/interop
updates packageDocumentation with example

Related #416
Fixes #422

Notes & open questions

feat: require content-type parser to set content-type
test: remove vFetch interop content-type assertions
docs: add content-type customization example

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation if necessary (this includes comments as well)
I have added tests that prove my fix is effective or that my feature works

fixes #422

SgtPooki

self review

SgtPooki · 2024-02-07T19:54:39Z

packages/verified-fetch/src/index.ts

@@ -112,6 +112,26 @@
 * const json = await resp.json()
 * ```
 *
+ * ### Custom content-type parsing
+ *
+ * By default, `@helia/verified-fetch` does not set the `Content-Type` header. This is because the `.json()`, `.text()`, `.blob()`, and `.arrayBuffer()` methods will usually work as expected. You can provide a `contentTypeParser` function to the `createVerifiedFetch` function to handle parsing the content type. The function you provide will be passed the first bytes we receive from the network.


We could link to #422 if desired.

Suggested change

* By default, `@helia/verified-fetch` does not set the `Content-Type` header. This is because the `.json()`, `.text()`, `.blob()`, and `.arrayBuffer()` methods will usually work as expected. You can provide a `contentTypeParser` function to the `createVerifiedFetch` function to handle parsing the content type. The function you provide will be passed the first bytes we receive from the network.

* By default, `@helia/verified-fetch` does not set the `Content-Type` header. This is because the `.json()`, `.text()`, `.blob()`, and `.arrayBuffer()` methods will usually work as expected. You can provide a `contentTypeParser` function to the `createVerifiedFetch` function to handle parsing the content type. The function you provide will be passed the first bytes we receive from the network. See https://github.com/ipfs/helia/issues/422 for our thoughts and reasoning.

SgtPooki · 2024-02-07T19:55:49Z

packages/verified-fetch/src/index.ts

+ *
+ * ```typescript
+ * import { createVerifiedFetch } from '@helia/verified-fetch'
+ * import { fileTypeFromBuffer } from '@sgtpooki/file-type'


not referencing mime-types because it only does recognition based on extension, and the troubles @2color mentioned with ipfs-examples/helia-examples#285.

Not referencing file-types because of the lack of browser support, that has been tested and verified in https://github.com/SgtPooki/file-type

SgtPooki · 2024-02-07T19:56:38Z

packages/verified-fetch/src/index.ts

+ *  routers: ['http://delegated-ipfs.dev'],
+ *  contentTypeParser: async (bytes) => {
+ *    // call to some magic-byte recognition library like magic-bytes, file-type, or your own custom byte recognition
+ *    return fileTypeFromBuffer(bytes)?.mime ?? 'application/octet-stream'


Suggested change

* return fileTypeFromBuffer(bytes)?.mime ?? 'application/octet-stream'

* return (await fileTypeFromBuffer(bytes))?.mime ?? 'application/octet-stream'

SgtPooki · 2024-02-07T19:57:36Z

packages/verified-fetch/src/index.ts

+   * bytes we receive from the network, and should return a string that will be used as the value for the `Content-Type`
+   * header in the response.
+   */
+  contentTypeParser?: ContentTypeParser


Do we want to add contentTypeParser to verifiedFetch options? I don't think we need to allow passing it for each call, but we could. LMK

Perhaps not in the first instance to keep it simple? We can always add it later.

SgtPooki · 2024-02-07T19:59:54Z

packages/verified-fetch/src/index.ts

+   * bytes we receive from the network, and should return a string that will be used as the value for the `Content-Type`
+   * header in the response.
+   */
+  contentTypeParser?: ContentTypeParser


Do we want to accept an options param here that allows for the VerifiedFetchInit options and just pass that through? That seems better and less prone to CreateVerifiedFetchWithOptions growing unnecessarily large.

I'm not sure, I think maybe we want to create a separation between the createVerifiedFetch options arg type and the VerifiedFetch class init arg type.

thats why I went with this initially. we could always update if we really need to, but i don't imagine createVerifiedFetch init args will vary much

…tream

achingbrain · 2024-02-08T12:51:50Z

packages/interop/src/verified-fetch-unixfs-dir.spec.ts

-  let verifiedFetch: Awaited<ReturnType<typeof createVerifiedFetch>>
+  let verifiedFetch: VerifiedFetch


This type is a bit simpler.

packages/verified-fetch/src/index.ts

achingbrain

LGTM.

I made a few changes, please comment if you disagree with them:

Split the content type parser tests into their own file
Defaulted to a Content-Type of application/octet-stream instead of not setting one - I think this would be more consistent with what a web server would do
Added tests that show how to use @sgtpooki/file-type and also magic-bytes.js as they both work in browsers
Allows the contentTypeParser to be sync as well as async, and to also return undefined in which case we fall back to application/octet-stream

Also fixed a few bugs:

The contentTypeParser option wasn't being passed through if you used createVerifiedFetch
The interop test had guessing the type of an image as image/png removed - the file is actually image/jpeg with a .png extension , I guess this was throwing the previous content-type guesser off - now the test tests for the correct mime type

achingbrain · 2024-02-08T13:03:46Z

~~We may wish to also pass the file name to contentTypeParser if it's available? This way if it's passed index.html it can just use the extension to return text/html instead of nothing.~~

This is done now ✅

SgtPooki

i'm good with the changes

SgtPooki · 2024-02-08T16:11:44Z

packages/interop/src/verified-fetch-unixfs-dir.spec.ts

+
+    it('can return an image content-type for unixfs pathed data', async () => {
+      const resp = await verifiedFetch('ipfs://QmbQDovX7wRe9ek7u6QXe9zgCXkTzoUSsTFJEkrYV1HrVR/1 - Barrel - Part 1.png')
+      // tediously this is actually a jpeg file with a .png extension


SgtPooki added 3 commits February 7, 2024 11:34

feat: require content-type parser to set content-type

c4f3355

fixes #422

test: remove vFetch interop content-type assertions

e31f907

docs: add content-type customization example

63d0530

SgtPooki requested a review from a team as a code owner February 7, 2024 19:53

SgtPooki linked an issue Feb 7, 2024 that may be closed by this pull request

feat: customizeable content type parsing in @helia/verified-fetch #422

Closed

SgtPooki commented Feb 7, 2024

View reviewed changes

deps: remove mime-types

abf9a0d

SgtPooki self-assigned this Feb 7, 2024

SgtPooki mentioned this pull request Feb 7, 2024

feat(verified-fetch): improve UX ipfs-examples/helia-examples#286

Merged

achingbrain added 3 commits February 8, 2024 13:30

chore: add more content type tests and default to application/octet-s…

ab4ecd1

…tream

chore: fix deps

1a291d7

chore: actually pass the content type parser through

f2e80c4

achingbrain reviewed Feb 8, 2024

View reviewed changes

2color reviewed Feb 8, 2024

View reviewed changes

packages/verified-fetch/src/index.ts Show resolved Hide resolved

achingbrain approved these changes Feb 8, 2024

View reviewed changes

achingbrain added 2 commits February 8, 2024 16:58

feat: pass filename to content type parser if available

03e9835

chore: add test for deep traversal filenames

e7e24da

SgtPooki commented Feb 8, 2024

View reviewed changes

achingbrain merged commit f58d467 into main Feb 8, 2024
18 checks passed

achingbrain deleted the 422-feat-customizeable-content-type-parsing-in-heliaverified-fetch branch February 8, 2024 16:39

achingbrain mentioned this pull request Feb 8, 2024

chore: release main #411

Merged

SgtPooki mentioned this pull request Apr 9, 2024

docs: fixing changelogs SgtPooki/helia#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: require content-type parser to set content-type #423

feat: require content-type parser to set content-type #423

SgtPooki commented Feb 7, 2024 •

edited

Loading

SgtPooki left a comment

SgtPooki Feb 7, 2024

SgtPooki Feb 7, 2024

SgtPooki Feb 7, 2024

SgtPooki Feb 7, 2024

achingbrain Feb 8, 2024

SgtPooki Feb 7, 2024

achingbrain Feb 8, 2024

SgtPooki Feb 8, 2024

achingbrain Feb 8, 2024

achingbrain left a comment •

edited

Loading

achingbrain commented Feb 8, 2024 •

edited

Loading

SgtPooki left a comment

SgtPooki Feb 8, 2024

achingbrain Feb 8, 2024

	* return fileTypeFromBuffer(bytes)?.mime ?? 'application/octet-stream'
	* return (await fileTypeFromBuffer(bytes))?.mime ?? 'application/octet-stream'

		let verifiedFetch: Awaited<ReturnType<typeof createVerifiedFetch>>
		let verifiedFetch: VerifiedFetch

feat: require content-type parser to set content-type #423

feat: require content-type parser to set content-type #423

Conversation

SgtPooki commented Feb 7, 2024 • edited Loading

Title

Description

Notes & open questions

Change checklist

SgtPooki 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

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

achingbrain left a comment • edited Loading

Choose a reason for hiding this comment

achingbrain commented Feb 8, 2024 • edited Loading

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki commented Feb 7, 2024 •

edited

Loading

achingbrain left a comment •

edited

Loading

achingbrain commented Feb 8, 2024 •

edited

Loading