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(fetch): add support for file urls in fetch #1239

Closed
wants to merge 1 commit into from
Closed
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
71 changes: 70 additions & 1 deletion lib/fetch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const { PassThrough, pipeline } = require('stream')
const { isErrored, isReadable } = require('../core/util')
const { kIsMockActive } = require('../mock/mock-symbols')
const { dataURLProcessor } = require('./dataURL')
const fs = require('fs')

/** @type {import('buffer').resolveObjectURL} */
let resolveObjectURL
Expand Down Expand Up @@ -882,7 +883,7 @@ async function schemeFetch (fetchParams) {
case 'file:': {
// For now, unfortunate as it is, file URLs are left as an exercise for the reader.
Copy link
Member

Choose a reason for hiding this comment

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

remove comment?

Copy link
Member Author

@Linkgoron Linkgoron Feb 17, 2022

Choose a reason for hiding this comment

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

That's from the spec itself, other comments also include the text from the spec itself.

// When in doubt, return a network error.
return makeNetworkError('not implemented... yet...')
return fileFetch.call(this, fetchParams)
}
case 'http:':
case 'https:': {
Expand Down Expand Up @@ -1990,4 +1991,72 @@ function httpNetworkFetch (
})
}

async function fileFetch (fetchParams) {
const { request } = fetchParams
const url = requestCurrentURL(request)
const context = this
Copy link
Member

Choose a reason for hiding this comment

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

why is context just not this?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's the same style that's used in other methods, so I wanted to keep it the same.

if (request.method !== 'GET') {
return makeNetworkError(`Fetching files only supports the GET method. Received ${request.method}`)
Copy link
Member

Choose a reason for hiding this comment

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

What do browsers do with HEAD?

Copy link
Member Author

@Linkgoron Linkgoron Feb 17, 2022

Choose a reason for hiding this comment

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

I looked at what Deno does, I even kept the message the same. I assume that Chrome behaves in a similar way, but I'll check it out.

}

let stream
let file

try {
file = await fs.promises.open(url)
if (context.terminated) {
if (context.terminated.aborted) {
throw new AbortError()
} else {
throw new Error('terminated')
}
}
stream = file.createReadStream({ autoClose: true })
} catch (originalError) {
if (stream) {
stream.destroy(originalError)
} else if (file) {
try {
await file.close()
} catch (closeError) {
return createAggregateNetworkError(originalError, closeError)
}
}
return makeNetworkError(originalError)
}

try {
function onTerminated () {
const aborted = context.terminated.aborted
if (aborted) {
response.aborted = true
stream.destroy(new AbortError())
} else {
stream.destroy(new TypeError('terminated'))
Copy link
Member

Choose a reason for hiding this comment

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

Not specific to these PR but these errors should have codes?

(That seems orthogonal though)

Copy link
Member Author

@Linkgoron Linkgoron Feb 17, 2022

Choose a reason for hiding this comment

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

I looked at what's done in other methods, so changes might need to get made in other places as well.

}
}
function streamClose () {
context.off('terminated', onTerminated)
}
const response = makeResponse({
status: 200,
statusText: 'OK',
headersList: [],
body: safelyExtractBody(stream)[0]
})
context.on('terminated', onTerminated)
stream.on('close', streamClose)
return response
} catch (err) {
stream.destroy(err)
return makeNetworkError(err)
}
}

function createAggregateNetworkError (originalError, secondError) {
const err = new AggregateError([originalError, secondError], originalError.message)
err.code = originalError.code
return makeNetworkError(err)
}

module.exports = fetch
22 changes: 22 additions & 0 deletions test/fetch/client-fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const { createServer } = require('http')
const { ReadableStream } = require('stream/web')
const { Blob } = require('buffer')
const { fetch, Response, Request, FormData, File, FileLike } = require('../..')
const { join } = require('path')
const { pathToFileURL } = require('url')

test('function signature', (t) => {
t.plan(2)
Expand Down Expand Up @@ -332,3 +334,23 @@ test('post FormData with File', (t) => {
t.ok(/filename123/.test(result))
})
})

test('fetch file', async (t) => {
t.plan(1)
const fileToRead = pathToFileURL(join(__dirname, '..', 'fixtures', 'file.text'))
const body = await fetch(fileToRead)
const text = await body.text()
t.equal(text, 'hello world')
})

test('fetch file does not exist', async (t) => {
t.plan(2)
const fileToRead = pathToFileURL(join(__dirname, '..', 'fixtures', 'does-not-exist.text'))
try {
await fetch(fileToRead)
t.fail('fetch should have thrown')
} catch (err) {
t.ok(err.cause)
t.equal(err.cause.code, 'ENOENT')
}
})
Copy link
Member

Choose a reason for hiding this comment

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

test AbortSignal?

Copy link
Member Author

@Linkgoron Linkgoron Feb 17, 2022

Choose a reason for hiding this comment

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

yes, tests are the main thing that's missing and why I've put this as a draft (also need to see what to do with headers)

1 change: 1 addition & 0 deletions test/fixtures/file.text
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello world