-
Notifications
You must be signed in to change notification settings - Fork 15
To use firebase in react native the dom api dependency needed to be removed #5
base: master
Are you sure you want to change the base?
Conversation
See: https://firebase.google.com/docs/reference/js/firebase.storage.Reference this basically removes the dependency on dom and allows this to be used on react native
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. I've been meaning to do this for a very long time but haven't gotten to it yet.
Just a few minor comments 👍
"firebase": "^4.4.0" | ||
"bs-platform": "^2.1.0", | ||
"firebase": "^4.4.0", | ||
"global": "^4.3.2" |
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.
What is this?
}, | ||
"dependencies": { | ||
"firebase": "^4.4.0" | ||
"bs-platform": "^2.1.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.
This should be a peer dependency and dev depedency, but not a depedency. It's a problem with packages being dependencies between projcets.
}, | ||
"keywords": [ | ||
"BuckleScript" | ||
], | ||
"license": "MIT", | ||
"peerDependencies": { | ||
"bs-platform": "^2.0.0", | ||
"bs-webapi": "git+https://github.com/viskahq/bs-webapi-incubator.git" | ||
"bs-platform": "^2.0.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.
Since the depedency (which should be dev) is 2.1.0 this should be that as well
@@ -109,7 +107,7 @@ module Storage = { | |||
[@bs.send] external path : (t, ~path: string) => t = ""; | |||
[@bs.send] | |||
external put : | |||
(t, ~data: Window.File.t, ~metadata: Js.t('a)=?, unit) => Js.Promise.t(UploadTask.t) = | |||
(t, ~data: Js.Array.t(int), ~metadata: Js.t('a)=?, unit) => Js.Promise.t(UploadTask.t) = |
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.
Maybe just have it 'a
since Js.Array of int is definitely not correct in the browser.
Firebase api in javascript has no such dependency either, it seems to be pretty platform agnostic.
Please test if this change will compile for you.
I'm following the firebase spec which says that the put data should be an array of ints, however I can imagine this will break stuff for you. Please tell me how to fix this properly then.
I'm not sure how you use this.