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

feat: dont use ipfs-utils fetch with upload progress by default #1240

Merged
merged 5 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/upload-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"exports": {
".": "./dist/src/index.js",
"./car": "./dist/src/car.js",
"./fetch-with-upload-progress": "./dist/src/fetch-with-upload-progress.js",
"./sharding": "./dist/src/sharding.js",
"./upload": "./dist/src/upload.js",
"./store": "./dist/src/store.js",
Expand Down
8 changes: 8 additions & 0 deletions packages/upload-client/src/fetch-with-upload-progress.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import ipfsUtilsFetch from 'ipfs-utils/src/http/fetch.js'
Copy link
Member

Choose a reason for hiding this comment

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

🙏 Pull out as a separate library that solves the issue or send a PR.


/**
* @type {import('./types.js').FetchWithUploadProgress}
*/
export const fetchWithUploadProgress = (url, init) => {
return ipfsUtilsFetch.fetch(url, init)
}
27 changes: 18 additions & 9 deletions packages/upload-client/src/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import { SpaceDID } from '@web3-storage/capabilities/utils'
import retry, { AbortError } from 'p-retry'
import { servicePrincipal, connection } from './service.js'
import { REQUEST_RETRIES } from './constants.js'
import fetchPkg from 'ipfs-utils/src/http/fetch.js'
const { fetch } = fetchPkg

/**
*
Expand Down Expand Up @@ -90,10 +88,9 @@ export async function add(
const responseAddUpload = result.out.ok

const fetchWithUploadProgress =
/** @type {(url: string, init?: import('./types.js').FetchOptions) => Promise<Response>} */ (
fetch
)
options.fetchWithUploadProgress || options.fetch || globalThis.fetch
Copy link
Member

Choose a reason for hiding this comment

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

Needs globalThis.fetch.bind(globalThis)

Copy link
Member

Choose a reason for hiding this comment

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

Just call it customFetch and type it as required?


let fetchDidCallUploadProgressCb = false
const res = await retry(
async () => {
try {
Expand All @@ -103,12 +100,14 @@ export async function add(
body: car,
headers: responseAddUpload.headers,
signal: options.signal,
onUploadProgress: options.onUploadProgress
? createUploadProgressHandler(
onUploadProgress: (status) => {
fetchDidCallUploadProgressCb = true
if (options.onUploadProgress)
createUploadProgressHandler(
responseAddUpload.url,
options.onUploadProgress
)
: undefined,
)(status)
},
// @ts-expect-error - this is needed by recent versions of node - see https://github.com/bluesky-social/atproto/pull/470 for more info
duplex: 'half',
})
Expand All @@ -128,6 +127,16 @@ export async function add(
}
)

if (!fetchDidCallUploadProgressCb && options.onUploadProgress) {
// the fetch implementation didn't support onUploadProgress
const carBlob = new Blob([car])
options.onUploadProgress({
total: carBlob.size,
Copy link
Member

Choose a reason for hiding this comment

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

are total and loaded here the same because this will be the only progress event sent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep! the only progress even for this car file, and all we know here is the fetch succeeded so it's 100% progress all at once

loaded: carBlob.size,
lengthComputable: false,
})
}

if (!res.ok) {
throw new Error(`upload failed: ${res.status}`)
}
Expand Down
22 changes: 20 additions & 2 deletions packages/upload-client/src/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {
FetchOptions,
FetchOptions as IpfsUtilsFetchOptions,
ProgressStatus as XHRProgressStatus,
} from 'ipfs-utils/src/types.js'
import { Link, UnknownLink, Version } from 'multiformats/link'
Expand Down Expand Up @@ -45,6 +45,16 @@ import {
UsageReportFailure,
} from '@web3-storage/capabilities/types'

type Override<T, R> = Omit<T, keyof R> & R

type FetchOptions = Override<
IpfsUtilsFetchOptions,
{
// `fetch` is a browser API and browsers don't have `Readable`
body: Exclude<IpfsUtilsFetchOptions['body'], import('node:stream').Readable>
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it excludes Readable from the allowed body types that come from ipfs-utils because that's not what other fetches will expect

}
>

export type {
FetchOptions,
StoreAdd,
Expand Down Expand Up @@ -186,7 +196,13 @@ export interface Connectable {
connection?: ConnectionView<Service>
}

export type FetchWithUploadProgress = (
url: string,
init?: FetchOptions
) => Promise<Response>

export interface UploadProgressTrackable {
fetchWithUploadProgress?: FetchWithUploadProgress
onUploadProgress?: ProgressFn
}

Expand All @@ -210,7 +226,9 @@ export interface RequestOptions
extends Retryable,
Abortable,
Connectable,
UploadProgressTrackable {}
UploadProgressTrackable {
fetch?: typeof globalThis.fetch
}

export interface ListRequestOptions extends RequestOptions, Pageable {}

Expand Down
34 changes: 31 additions & 3 deletions packages/upload-client/test/store.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { serviceSigner } from './fixtures.js'
import { randomCAR } from './helpers/random.js'
import { mockService } from './helpers/mocks.js'
import { validateAuthorization } from './helpers/utils.js'
import { fetchWithUploadProgress } from '../src/fetch-with-upload-progress.js'

describe('Store.add', () => {
it('stores a DAG with the service', async () => {
Expand Down Expand Up @@ -62,25 +63,52 @@ describe('Store.add', () => {
channel: server,
})

let loaded = 0
/** @type {import('../src/types.js').ProgressStatus[]} */
const progress = []
const carCID = await Store.add(
{ issuer: agent, with: space.did(), proofs, audience: serviceSigner },
car,
{
connection,
onUploadProgress: (status) => {
assert(typeof status.loaded === 'number' && status.loaded > 0)
loaded = status.loaded
progress.push(status)
},
fetchWithUploadProgress,
}
)

assert(service.store.add.called)
assert.equal(service.store.add.callCount, 1)
assert.equal(loaded, 225)
assert.equal(
progress.reduce((max, { loaded }) => Math.max(max, loaded), 0),
225
)

assert(carCID)
assert.equal(carCID.toString(), car.cid.toString())

// make sure it can also work without fetchWithUploadProgress
/** @type {import('../src/types.js').ProgressStatus[]} */
let progressWithoutUploadProgress = []
const addedWithoutUploadProgress = await Store.add(
{ issuer: agent, with: space.did(), proofs, audience: serviceSigner },
car,
{
connection,
onUploadProgress: (status) => {
progressWithoutUploadProgress.push(status)
},
}
)
assert.equal(addedWithoutUploadProgress.toString(), car.cid.toString())
assert.equal(
progressWithoutUploadProgress.reduce(
(max, { loaded }) => Math.max(max, loaded),
0
),
225
)
})

it('throws for bucket URL client error 4xx', async () => {
Expand Down
Loading