-
Notifications
You must be signed in to change notification settings - Fork 12
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
Api bzz react native #98
Conversation
Thanks for your PR! |
damn nvm :) I was running the tests on another tab with v8.9.0. |
I think some tests probably have dangling timers (notably when testing subscriptions) that aren't cleared when an error is thrown, I'll try to go through these when I have the chance. |
Hi, I was thinking about how to move forward. I can create a simple react-native application, where we can run functionally the same tests as in the browser test suite, I'm just wondering what would be the best setup from the repository and the CI/CD point of view. I can imagine the following setups:
The first one is the most straightforward, but I'm not sure if you want to put a relatively heavy sub-project (the most basic react-native app) to this repo. |
Hey, thanks for looking into this! I agree that the first option is likely the simplest setup. For CI currently we're using Circle but it doesn't have a free macOS plan (which I assume is needed to test iOS?), so I'm thinking about switching to Travis or Azure Pipelines, do you have any recommendation please? |
We are using Travis, sometimes we saw XCode builds fail in a non-reproducible way, but it was rare, and haven't seen it in a while either. I'll have a look at creating the app in the coming weeks. |
Great, thanks! Also FYI I started rewriting Erebos in TypeScript so it may affect your changes, here is the current changelog: https://github.com/MainframeHQ/erebos/blob/typescript/CHANGELOG.md#v090-unreleased |
Sure! Happy to hear you are moving to TypeScript! |
Thanks! |
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 for the PR!
Beyond the few questions below, it seems the types/index.d.ts
from api-bzz-browser
has been moved to api-bzz-react-native
, while both packages should probably have it, could you fix this as well please?
Thanks!
"universal-url": "^2.0.0" | ||
}, | ||
"devDependencies": { | ||
"flow-bin": "^0.101.0" |
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.
Could you remove this dependency now that the project uses TypeScript please?
export class Bzz extends BaseBzz<Response> { | ||
public constructor(config: BzzConfig) { | ||
const { url, ...cfg } = config | ||
super(window.fetch.bind(window), { ...cfg, url: new URL(url).href }) |
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.
Shouldn't this use global
instead of window
?
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.
Not sure if this is needed, fetch is available globally in react-native, should we just pass in fetch
?
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'm not sure in RN but on the Web it needed to be bound to the proper context, there was some issue without.
Thanks! |
Changes
Implements partial support for react-native, for the
bzz
functionality in particular. Related issue #52There are 2 differences compared to
api-bzz-browser
:URL
type is used for other purposes in react-native, so in the implementation we have to depend on a URL type provided byuniversal-url
Blob
andFormData
has some differences in react-native and typing issues (both in flow and TypeScript), this had to be resolved as wellTodo
Run tests in detox, or other method which is closer to react-native then node / browser.
This is a draft PR, I opened it to make further discussions easier.
I had some tests failing locally:
FAIL tests/timeline.js
● Test suite failed to run
FAIL tests/api-bzz-node.js
● api-bzz-node › encountered a declaration exception