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: React Native support with fetchOption and callOptions #653
Changes from 12 commits
724d8fc
a9e7d5d
2ad9a20
2f07956
3298d3c
8b6b183
425f0b1
1be96e0
b1178c4
be9bf07
4f659bd
ea15c11
56071af
b7c8e50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ jobs: | |
spec: | ||
- '0.16.1' | ||
node: | ||
- 12 | ||
- 14 | ||
- 16 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
{ | ||
"/api": { | ||
"target": "http://localhost:8000/", | ||
"target": "http://localhost:4943/", | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,13 @@ export interface HttpAgentOptions { | |
// A surrogate to the global fetch function. Useful for testing. | ||
fetch?: typeof fetch; | ||
|
||
// Additional options to pass along to fetch. Will not override fields that | ||
// the agent already needs to set | ||
fetchOptions?: Record<string, unknown>; | ||
|
||
// Additional options to pass along to fetch for the call API. | ||
callOptions?: Record<string, unknown>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might eventually make sense to include one or two unit tests to ensure that these options can be used and overridden as expected. The implementation looks good, so this would just be to minimize the chance that someone doesn't accidentally break the expected behavior a few years from now (since a regression might cause some very subtle bugs that could be difficult to identify otherwise). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've attempted to create a minor unit test here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, could be something like that. What Node.js version are you using? The unit test works as expected on my laptop (equivalent to the CI output). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed some fixes for the unit test to hopefully save you a bit of time. Feel free to make any changes or improvements as you see fit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks so much! I was using node version 16.17.0. (I'm still not sure how to get the unit test to work locally.) But I'm glad this fix works! |
||
|
||
// The host to use for the client. By default, uses the same host as | ||
// the current page. | ||
host?: string; | ||
|
@@ -162,6 +169,8 @@ export class HttpAgent implements Agent { | |
private readonly _pipeline: HttpAgentRequestTransformFn[] = []; | ||
private _identity: Promise<Identity> | null; | ||
private readonly _fetch: typeof fetch; | ||
private readonly _fetchOptions?: Record<string, unknown>; | ||
private readonly _callOptions?: Record<string, unknown>; | ||
private _timeDiffMsecs = 0; | ||
private readonly _host: URL; | ||
private readonly _credentials: string | undefined; | ||
|
@@ -181,6 +190,8 @@ export class HttpAgent implements Agent { | |
this._credentials = options.source._credentials; | ||
} else { | ||
this._fetch = options.fetch || getDefaultFetch() || fetch.bind(global); | ||
this._fetchOptions = options.fetchOptions; | ||
this._callOptions = options.callOptions; | ||
} | ||
if (options.host !== undefined) { | ||
if (!options.host.match(/^[a-z]+:/) && typeof window !== 'undefined') { | ||
|
@@ -301,6 +312,7 @@ export class HttpAgent implements Agent { | |
|
||
const request = this._requestAndRetry(() => | ||
this._fetch('' + new URL(`/api/v2/canister/${ecid.toText()}/call`, this._host), { | ||
...this._callOptions, | ||
...transformedRequest.request, | ||
body, | ||
}), | ||
|
@@ -386,6 +398,7 @@ export class HttpAgent implements Agent { | |
const body = cbor.encode(transformedRequest.body); | ||
const response = await this._requestAndRetry(() => | ||
this._fetch('' + new URL(`/api/v2/canister/${canister.toText()}/query`, this._host), { | ||
...this._fetchOptions, | ||
...transformedRequest.request, | ||
body, | ||
}), | ||
|
@@ -445,6 +458,7 @@ export class HttpAgent implements Agent { | |
const response = await this._fetch( | ||
'' + new URL(`/api/v2/canister/${canister}/read_state`, this._host), | ||
{ | ||
...this._fetchOptions, | ||
...transformedRequest.request, | ||
body, | ||
}, | ||
|
@@ -497,7 +511,7 @@ export class HttpAgent implements Agent { | |
: {}; | ||
|
||
const response = await this._requestAndRetry(() => | ||
this._fetch('' + new URL(`/api/v2/status`, this._host), { headers }), | ||
this._fetch('' + new URL(`/api/v2/status`, this._host), { headers, ...this._fetchOptions }), | ||
); | ||
|
||
return cbor.decode(await response.arrayBuffer()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this string value is being used so many times here, perhaps we could use this PR as an opportunity to introduce something like
const HTTP_AGENT_HOST = 'localhost:4943';
near the top of the file, and then pointing each of these usages to this constant value. This would simplify the process of making similar changes in the future (such as if we wanted to determine the port from a config file, etc).