feat: support http range header #10
Conversation
|
On mobile but wanted to give a shout that ive got a refactor going that cleans this up a bunch |
Done a first pass, will look again after the refactor lands. |
| public get fileSize (): number | null | undefined { | ||
| return this._fileSize | ||
| } |
There was a problem hiding this comment.
We don't have to, but it will help prevent future developers from mistakenly trying to set things they shouldn't without understanding the implications. Setters and getters help explicitly state, in the way the code was implemented, that specific things have to be done when a value is applied, whereas a public non-readonly fileSize implies that it can be called at any time.
e.g. we call the setOffsets when setting fileSize. Without the property being protected, it can be called by anyone in the codebase. When this property isn't wrapped with a getter, and is public, it becomes available as a setter externally. When in reality, the fileSize property should not be publicly writeable, but to make it public and remove setter/getter, we have to make it so. We could use readonly and then override protections in the class.. but that feels like a lot of work for encapsulating that protection that we get by doing the conventional thing.
aside: IMHO setting and getting are cleaner than "setFileSize" and ".filesize" or "getFilesize"
Also... microsoft/TypeScript#37487
I addressed this with 46dc133 (#10), but we should think about reverting it.
…port FYI: some tests were failing after `npm run reset` and `npm run test` was run. I had to fix them manually.
|
FYI: some tests were failing when merging main into byte-range PR due to new ipns things. changes at |
Co-authored-by: Alex Potsides <alex@achingbrain.net>
| * - Content-Range: <unit> <byteStart>-<byteEnd>/<byteSize> | ||
| * - Content-Range: <unit> <byteStart>-<byteEnd>/‍* | ||
| */ | ||
| // - Content-Range: <unit> */<byteSize> // this is purposefully not in jsdoc block |
There was a problem hiding this comment.
@achingbrain is this better than all as single line comments?
| } catch (err) { | ||
| this.log.error('error parsing resource %s', resource, err) | ||
|
|
||
| return badRequestResponse('Invalid resource') |
There was a problem hiding this comment.
Less throwing, more bad fetch responses
|
commenting elsewhere since the thread (#10 (comment)) was collapsed: we could solve both concerns by setting internalFileSize so we know which value we set vs consumers.. but that could get confusing. i'll leave as is for now. i've got _isRangeRequest updated locally, and have isValidRangeRequest updated as well. So now we have: inaccessable properties outside the ByteRangeContext class:
|
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
* feat: implement new ipns record&answer properties * fix: parseUrlString response includes defined ttl, set ttl if ipnsCached * test: fix firefox failure * feat: support http range header (#10) * chore: limit body parameters to the types used * chore: add response-header helper and tests * feat: add range header parsing support * feat: verified-fetch supports range-requests * test: fix dns test asserting test failure since we are catching it now * fix: return 500 error when streaming unixfs content throws * fix: cleanup code and unexecuting tests hiding errors * chore: some cleanup and code coverage * tmp: most things working * fix: stream slicing and test correctness * chore: fixed some ByteRangeContext tests * test: add back header helpers * fix: unixfs tests are passing * fix: range-requests on raw content * feat: tests are passing moved transform stream over to https://github.com/SgtPooki/streams * chore: log string casing * chore: use 502 response instead of 500 * chore: use libp2p/interface for types in src * chore: failing to create range resp logs error * chore: Apply suggestions from code review * chore: fix broken tests from github PR patches (my own) * chore: re-enable stream tests for ByteRangeContext * chore: clean up getBody a bit * chore: ByteRangeContext getBody cleanup * chore: apply suggestions from code review Co-authored-by: Alex Potsides <alex@achingbrain.net> * fix: getSlicedBody uses correct types * chore: remove extra stat call * chore: fix jsdoc with '*/' * chore: fileSize is public property, but should not be used * test: fix blob comparisons that broke or were never worjing properly * chore: Update byte-range-context.ts Co-authored-by: Alex Potsides <alex@achingbrain.net> * chore: jsdoc cleanup * Revert "chore: fileSize is public property, but should not be used" This reverts commit 46dc133. * chore: jsdoc comments explaining .fileSize use * chore: isRangeRequest is public * chore: getters/setters update * chore: remove unnecessary _contentRangeHeaderValue * chore: ByteRangeContext uses setFileSize and getFileSize * chore: remove .stat changes that are no longer needed --------- Co-authored-by: Alex Potsides <alex@achingbrain.net> * chore(release): 1.2.0 [skip ci] ## @helia/verified-fetch [1.2.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-1.1.3...@helia/verified-fetch-1.2.0) (2024-03-15) ### Features * support http range header ([#10](#10)) ([9f5078a](9f5078a)) ### Trivial Changes * fix build ([#22](#22)) ([01261fe](01261fe)) * chore(release): 1.7.0 [skip ci] ## @helia/verified-fetch-interop [1.7.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-interop-1.6.0...@helia/verified-fetch-interop-1.7.0) (2024-03-15) ### Dependencies * **@helia/verified-fetch:** upgraded to 1.2.0 * chore: apply pr comments * fix: some ipns ttl precision cleanup --------- Co-authored-by: Alex Potsides <alex@achingbrain.net> Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
* fix: set cache-control header conditionally Fixes #17 * chore: bump deps * feat: implement new ipns record&answer properties (#23) * feat: implement new ipns record&answer properties * fix: parseUrlString response includes defined ttl, set ttl if ipnsCached * test: fix firefox failure * feat: support http range header (#10) * chore: limit body parameters to the types used * chore: add response-header helper and tests * feat: add range header parsing support * feat: verified-fetch supports range-requests * test: fix dns test asserting test failure since we are catching it now * fix: return 500 error when streaming unixfs content throws * fix: cleanup code and unexecuting tests hiding errors * chore: some cleanup and code coverage * tmp: most things working * fix: stream slicing and test correctness * chore: fixed some ByteRangeContext tests * test: add back header helpers * fix: unixfs tests are passing * fix: range-requests on raw content * feat: tests are passing moved transform stream over to https://github.com/SgtPooki/streams * chore: log string casing * chore: use 502 response instead of 500 * chore: use libp2p/interface for types in src * chore: failing to create range resp logs error * chore: Apply suggestions from code review * chore: fix broken tests from github PR patches (my own) * chore: re-enable stream tests for ByteRangeContext * chore: clean up getBody a bit * chore: ByteRangeContext getBody cleanup * chore: apply suggestions from code review Co-authored-by: Alex Potsides <alex@achingbrain.net> * fix: getSlicedBody uses correct types * chore: remove extra stat call * chore: fix jsdoc with '*/' * chore: fileSize is public property, but should not be used * test: fix blob comparisons that broke or were never worjing properly * chore: Update byte-range-context.ts Co-authored-by: Alex Potsides <alex@achingbrain.net> * chore: jsdoc cleanup * Revert "chore: fileSize is public property, but should not be used" This reverts commit 46dc133. * chore: jsdoc comments explaining .fileSize use * chore: isRangeRequest is public * chore: getters/setters update * chore: remove unnecessary _contentRangeHeaderValue * chore: ByteRangeContext uses setFileSize and getFileSize * chore: remove .stat changes that are no longer needed --------- Co-authored-by: Alex Potsides <alex@achingbrain.net> * chore(release): 1.2.0 [skip ci] ## @helia/verified-fetch [1.2.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-1.1.3...@helia/verified-fetch-1.2.0) (2024-03-15) ### Features * support http range header ([#10](#10)) ([9f5078a](9f5078a)) ### Trivial Changes * fix build ([#22](#22)) ([01261fe](01261fe)) * chore(release): 1.7.0 [skip ci] ## @helia/verified-fetch-interop [1.7.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-interop-1.6.0...@helia/verified-fetch-interop-1.7.0) (2024-03-15) ### Dependencies * **@helia/verified-fetch:** upgraded to 1.2.0 * chore: apply pr comments * fix: some ipns ttl precision cleanup --------- Co-authored-by: Alex Potsides <alex@achingbrain.net> Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net> * chore: add matchUrlGroups typeguard * chore: remove unnecessary headerValue != null check * test: remove unnecessary redefinition of verifiedFetch --------- Co-authored-by: Daniel N <2color@users.noreply.github.com> Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com> Co-authored-by: Alex Potsides <alex@achingbrain.net> Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
## @helia/verified-fetch [1.3.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-1.2.1...@helia/verified-fetch-1.3.0) (2024-03-21) ### Features * abort signals are respected ([#26](#26)) ([30148fe](30148fe)) ### Bug Fixes * set cache-control header correctly ([#19](#19)) ([114f3a4](114f3a4)), closes [#17](#17) [#23](#23) [#10](#10)
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Title
Add Range header support.
Description
Fixes #9
Notes & open questions
handling ReadableStreamstests, but these are covered byrange requeststests.. going to try to fix quickly and if I can't i'll remove them.Change checklist