-
Notifications
You must be signed in to change notification settings - Fork 793
Remove polyfills #357
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
base: master
Are you sure you want to change the base?
Remove polyfills #357
Conversation
is this PR tested? should i merge it into https://github.com/Frekansapp/rn-fetch-blob here? |
I have only tested the parts used by our apps. So, I cannot confirm if json stream integration works as expected. Also, slicing blobs stored native side (as opposed to in js memory) might act differently; uncertain, as I do not use said feature. |
@alpha0010 What do you think about this solution? #183 (comment) |
You didn't remove this interface. Can this lines cause problems? Lines 26 to 35 in 979810b
|
You are right; that would cause incorrect checks for typescript users. |
Would result in undefined usage. The name of the native module and (recommended name for the) js module are unfortunately the same. However, |
Looking good! Is this not ready to use yet? |
I would definitely love to remove these polyfills if possible, it seems we need more testing to confirm if anything will be broken by removing them |
@alpha0010 @Frekansapp @ThaJay @Traviskn will this be the breaking changes, as most of the people using the |
@sadafk831 Thank you for communicating the reasoning behind this. |
Fixes #183