Skip to content

Update Blob.js#16585

Closed
sm2017 wants to merge 5 commits intofacebook:masterfrom
sm2017:patch-2
Closed

Update Blob.js#16585
sm2017 wants to merge 5 commits intofacebook:masterfrom
sm2017:patch-2

Conversation

@sm2017
Copy link
Copy Markdown
Contributor

@sm2017 sm2017 commented Oct 29, 2017

Motivation

If you run the following code

fetch('https://mywebsite.com/path')
      .then((response) => response.arrayBuffer())

TypeError: response.arrayBuffer is not a function

The reason is fetch use whatwg-fetch and we have https://github.com/github/fetch/blob/master/fetch.js#L240-L265 and support.blob is false because https://github.com/github/fetch/blob/master/fetch.js#L16

new Blob() throw the following error

TypeError: Cannot read property 'forEach' of undefined
at new Blob (node_modules\react-native\Libraries\Blob\Blob.js:95)
at node_modules\whatwg-fetch\fetch.js:12
at a (node_modules\whatwg-fetch\fetch.js:10)
at node_modules\whatwg-fetch\fetch.js:487
at loadModuleImplementation (node_modules\metro-bundler\src\Resolver\polyfills\require.js:178)
at guardedLoadModule (node_modules\metro-bundler\src\Resolver\polyfills\require.js:130)
at _require (node_modules\metro-bundler\src\Resolver\polyfills\require.js:110)
at node_modules\react-native\Libraries\Network\fetch.js:17
at loadModuleImplementation (node_modules\metro-bundler\src\Resolver\polyfills\require.js:178)
at guardedLoadModule (node_modules\metro-bundler\src\Resolver\polyfills\require.js:123)

I change constructor to constructor(parts: Array<Blob>=[], options: any) to fix issue

Test Plan

N?A

Release Notes

[GENERAL] [BUGFIX] [Blob] - Fix new Blob() issue

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 29, 2017
@pull-bot
Copy link
Copy Markdown

pull-bot commented Oct 29, 2017

@facebook-github-bot label Android

Generated by 🚫 dangerJS

@shergin
Copy link
Copy Markdown
Contributor

shergin commented Oct 30, 2017

Okay... but in the specification the first argument is not optional.
https://developer.mozilla.org/en-US/docs/Web/API/Blob/Blob

@sm2017
Copy link
Copy Markdown
Contributor Author

sm2017 commented Oct 30, 2017

@shergin http://jsfiddle.net/thmqwy8t/
It's seems the document of mozilla must be updated

@sm2017
Copy link
Copy Markdown
Contributor Author

sm2017 commented Nov 2, 2017

@shergin Can you please accept this PR?

@ide
Copy link
Copy Markdown
Contributor

ide commented Nov 9, 2017

Reading the W3C spec this looks like it has the right behavior: https://w3c.github.io/FileAPI/#constructorBlob. It'd be nice to have a unit test (just for this case), want to add one?

Copy link
Copy Markdown
Contributor

@ide ide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit + request for simple unit test

Comment thread Libraries/Blob/Blob.js Outdated
* Reference: https://developer.mozilla.org/en-US/docs/Web/API/Blob/Blob
*/
constructor(parts: Array<Blob>, options: any) {
constructor(parts: Array<Blob>=[], options: any) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces around =

@sm2017
Copy link
Copy Markdown
Contributor Author

sm2017 commented Nov 13, 2017

@ide I put spaces , Just tell me where must I create unit test , I don't find correct path

@ide
Copy link
Copy Markdown
Contributor

ide commented Nov 14, 2017

See other examples in this repo -- make tests directories that are colocated with the files under test.

@sm2017
Copy link
Copy Markdown
Contributor Author

sm2017 commented Nov 15, 2017

@ide unit test added b4a471b

@sm2017
Copy link
Copy Markdown
Contributor Author

sm2017 commented Dec 6, 2017

@ide @shergin reply please

Comment thread Libraries/Blob/__tests__/Blob-test.js Outdated
blob = false;
}

expect(blob).toBe(true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you write this as expect(() => new Blob()).not.toThrow()?

@sm2017
Copy link
Copy Markdown
Contributor Author

sm2017 commented Dec 12, 2017

@ide Done

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@sm2017 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@ide
Copy link
Copy Markdown
Contributor

ide commented Jan 11, 2018

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 11, 2018
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ide is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added cla signed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 12, 2018
@hramos
Copy link
Copy Markdown
Contributor

hramos commented Jan 12, 2018

Here's the test that failed:

Libraries/Blob/__tests__/Blob-test.js

Blob construct
    expect(function).not.toThrow()
    
    Expected the function not to throw an error.
    Instead, it threw:
      ReferenceError: Blob is not defined
         \033[90m 12 | 
         \033[90m 13 | test('Blob construct', () => {
        >\033[90m 14 |   expect(() => new Blob()).not.toThrow();
         \033[90m 15 | });
         \033[90m 16 | 
          
          at react-native-github/Libraries/Blob/__tests__/Blob-test.js:14:23
          at Object.<anonymous> (node_modules/expect/build/to_throw_matchers.js:43:7)
          at Object.throwingMatcher [as toThrow] (node_modules/expect/build/index.js:215:24)
          at Object.<anonymous> (react-native-github/Libraries/Blob/__tests__/Blob-test.js:14:48)
     \033[90m 12 | 
     \033[90m 13 | test('Blob construct', () => {
    >\033[90m 14 |   expect(() => new Blob()).not.toThrow();
     \033[90m 15 | });
     \033[90m 16 | 
      
      at Object.<anonymous> (Libraries/Blob

@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
@react-native-bot react-native-bot added Bug Fix 🐛 Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018
@hramos
Copy link
Copy Markdown
Contributor

hramos commented Jul 19, 2018

Seems abandoned.

@hramos hramos closed this Jul 19, 2018
@hramos hramos added Merged This PR has been merged. and removed Import Failed labels Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Ran Commands One of our bots successfully processed a command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants