feat: React Native support with fetchOption and callOptions #653
Conversation
…l included the logic: - `this._fetchOptions = options.source._fetchOptions || options.fetchOptions;` - `this._callOptions = options.source._callOptions || options.callOptions;`
…Options and this._callOptions will automatically get fromm provided options.
…l included the logic: - `this._fetchOptions = options.source._fetchOptions || options.fetchOptions;` - `this._callOptions = options.source._callOptions || options.callOptions;`
…Options and this._callOptions will automatically get fromm provided options.
… into kyle/fetchOptions
|
@krpeacock
|
-made changes mainly inside tests and yml setup
…1 does not support node 12.
| @@ -313,26 +313,26 @@ test('use anonymous principal if unspecified', async () => { | |||
|
|
|||
| describe('getDefaultFetch', () => { | |||
| it("should use fetch from window if it's available", async () => { | |||
| const generateAgent = () => new HttpAgent({ host: 'localhost:8000' }); | |||
| const generateAgent = () => new HttpAgent({ host: 'localhost:4943' }); | |||
There was a problem hiding this comment.
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).
| 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.
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.
I've attempted to create a minor unit test here.
There is a repeated error that shows (node:2473) V8: /home/runner/work/agent-js/agent-js/node_modules/borc/src/decoder.asm.js:3 Linking failure in asm.js: Unexpected stdlib member.
Not sure but this might be due to issue here of jest globals and node globals differing. jestjs/jest#2549.
There was a problem hiding this comment.
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.
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.
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!
Description
For React Native project, to create new fetch settings that work with React-Native-Fetch-API polyfill.
When source isn't provided in constructing the HttpAgent, this._fetchOptions and this._callOptions will automatically pull from provided options. Currently this is the only place where fetchOptions and callOptions will be set.