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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
724d8fc
experimental changes
krpeacock a9e7d5d
added callOptions, and changed assignment logic of this._fetchOptions
wackyleo459 2ad9a20
Typo error. In case options.source is correctly implemented I've stil…
wackyleo459 2f07956
When source isn't provided in constructing the HttpAgent, this._fetch…
wackyleo459 3298d3c
experimental changes
krpeacock 8b6b183
added callOptions, and changed assignment logic of this._fetchOptions
wackyleo459 425f0b1
Typo error. In case options.source is correctly implemented I've stil…
wackyleo459 1be96e0
When source isn't provided in constructing the HttpAgent, this._fetch…
wackyleo459 b1178c4
Merge branch 'kyle/fetchOptions' of https://github.com/dfinity/agent-…
wackyleo459 be9bf07
Migration Change from port 8000 to 4943 for new DFX 0.12.0
wackyleo459 4f659bd
Node 12 support deprecated, along with recent latest npm version 9.1.…
wackyleo459 ea15c11
Merge branch 'sueann/fetchOptions' into kyle/fetchOptions
wackyleo459 56071af
Unit test added for agent/http.test.ts
wackyleo459 b7c8e50
Test both call and query behavior
rvanasa File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ jobs: | |
spec: | ||
- '0.16.1' | ||
node: | ||
- 12 | ||
- 14 | ||
- 16 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
{ | ||
"/api": { | ||
"target": "http://localhost:8000/", | ||
"target": "http://localhost:4943/", | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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!