Skip to content

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Remove polyfills #357

wants to merge 1 commit into from

Conversation

alpha0010
Copy link

Fixes #183

@alpha0010 alpha0010 mentioned this pull request May 8, 2019
@erennyuksell
Copy link

is this PR tested? should i merge it into https://github.com/Frekansapp/rn-fetch-blob here?

@alpha0010
Copy link
Author

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.

@erennyuksell
Copy link

@alpha0010 What do you think about this solution? #183 (comment)

@erennyuksell
Copy link

erennyuksell commented Jun 13, 2019

You didn't remove this interface. Can this lines cause problems?

rn-fetch-blob/index.d.ts

Lines 26 to 35 in 979810b

export interface Polyfill {
Blob: PolyfillBlob;
File: PolyfillFile;
XMLHttpRequest: PolyfillXMLHttpRequest;
ProgressEvent: PolyfillProgressEvent;
Event: PolyfillEvent;
FileReader: PolyfillFileReader;
Fetch: PolyfillFetch;
}

@alpha0010
Copy link
Author

You are right; that would cause incorrect checks for typescript users.

@alpha0010
Copy link
Author

@alpha0010 What do you think about this solution? #183 (comment)

Would result in undefined usage. The name of the native module and (recommended name for the) js module are unfortunately the same. However, index.js exports a js object with methods that do not exist on the native module, but are required by the polyfill.

@ThaJay
Copy link

ThaJay commented Jul 23, 2019

Looking good! Is this not ready to use yet?

@Traviskn
Copy link

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

@Traviskn Traviskn added the help wanted Extra attention is needed label Sep 26, 2019
@sadafk831
Copy link

@alpha0010 @Frekansapp @ThaJay @Traviskn will this be the breaking changes, as most of the people using the Firebase still use the fix of global.XMLHttpRequest = RNFetchBlob.polyfill.XMLHttpRequest
So once the polyfills are removed, these will break there app

@ThaJay
Copy link

ThaJay commented Nov 11, 2019

@sadafk831 Thank you for communicating the reasoning behind this.
There have been multiple solutions proposed in the relevant issue: #183 but nobody has stepped up to fix it thus far.

@alpha0010
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require cycle
5 participants