Skip to content

update base64-js version. follow up #6961 - #6937

Closed
zxcpoiu wants to merge 1 commit into
react:masterfrom
zxcpoiu:patch-2
Closed

update base64-js version. follow up #6961#6937
zxcpoiu wants to merge 1 commit into
react:masterfrom
zxcpoiu:patch-2

Conversation

@zxcpoiu

@zxcpoiuzxcpoiu commented Apr 12, 2016

Copy link
Copy Markdown
Contributor

Motivation
This is originally a follow up of PR #6327 by @dewe and @jeremyong
And @mkonicek indicated this should split into a separate PR.

Now @philikon takes over #6327 as #6961

Test Plan:
According to #6961 and #6889
Load WebSocketExample in UIExplorer app and start websocket test server script and test sending binary data on both iOS and Android

@facebook-github-bot

Copy link
Copy Markdown
Contributor

By analyzing the blame information on this pull request, we identified @lindskogen, @cpojer and @sahrens to be potential reviewers.

@facebook-github-botfacebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 12, 2016
@christopherdro

christopherdro commented Apr 16, 2016

Copy link
Copy Markdown
Contributor

@mkonicek Can you check on this since you guys update the internal fb npm cache manually.

@zxcpoiuzxcpoiu changed the title update base64-js version. follow up #6327update base64-js version. follow up #6961Apr 19, 2016
@zxcpoiu

Copy link
Copy Markdown
ContributorAuthor

note: this PR now depends on #6961
and needs @philikon to review

@philikon

Copy link
Copy Markdown
Contributor

note: this PR now depends on #6961

I don't see why that's the case.

Comment threadpackage.json Outdated

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.

Why not ^1.1.0 as that version seems to have a bunch of improvements over 1.0.4 (whereas 1.1.1 and 1.1.2 seem to only be about aesthetics).

@philikon

Copy link
Copy Markdown
Contributor

Pro-tip: make sure you explain the motivation for your PR. You're currently not giving any, instead pointing to other PRs which also don't give motivation. If you want to make it easier for reviewers, and quicker for you, I suggest editing your PR note to include a small note about why you want this change. Also please don't forget a test plan.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@zxcpoiu updated the pull request.

@zxcpoiu

Copy link
Copy Markdown
ContributorAuthor

@philikon
thanks for your tips. update as you described.

why this PR exist and described as a follow up of #6961 are because:

  1. original author @dewe add in his commit da084b7 in Add support for sending binary data in websockets. #6327
  2. later Add support for sending binary data in websockets. #6327 hangs for a while and everybody seems busy, so I just helped to complete the rest based on @dewe 's patch --- this PR.
  3. then @philikon takes over whole PR Add support for sending binary data in websockets. #6327 into Add support for sending binary data in websockets #6961

This PR should be closed if no one needed

  1. my motivation is purely hope awesome PRs won't hang.
  2. follow up author @philikon in Add support for sending binary data in websockets #6961 confirmed Add support for sending binary data in websockets #6961 works on bas64-js 0.0.8 ( not necessary to upgrade )

If no one thinks this is necessary, then this PR should be closed.
How do you think @dewe ?

thanks again for all the great jobs.

@dewe

dewe commented Apr 20, 2016

Copy link
Copy Markdown

I can't recall why this was upgrade was needed. Since @philikon is handling things in the other PRs, this one should definitely be closed.

@zxcpoiu

Copy link
Copy Markdown
ContributorAuthor

Thanks @dewe
#6961 has been shipped, thank you 💯

@mkonicek@philikon
I think it is better to upgrade base64js to 1.x version if we using it as react native core.

do you think this is good to go?

@mkonicek

Copy link
Copy Markdown
Contributor

Upgrading sounds good. I'll need to merge this manually into our internal node_modules store though.

@ghost

ghost commented May 5, 2016

Copy link
Copy Markdown

@cpojer would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@ghostghost 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 Jul 12, 2016
This is a follow up of PR react#6327
by @dewe and @jeremyong
as @mkonicek indicated this should split into a separate PR
@ghostghost 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 Jul 14, 2016
@javache

Copy link
Copy Markdown
Contributor

@facebook-github-bot import

@ghost

ghost commented Aug 4, 2016

Copy link
Copy Markdown

Thanks for importing.

@ghostghost 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 Aug 4, 2016
@ghost

Copy link
Copy Markdown

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @cpojer as a potential reviewer. Could you take a look please or cc someone with more context?

@ghostghost 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 Aug 13, 2016
@mkonicek

Copy link
Copy Markdown
Contributor

Took a while to get back to this one :)

@facebook-github-bot import

@ghost

ghost commented Sep 9, 2016

Copy link
Copy Markdown

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghostghost 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 Sep 9, 2016
@ghostghost closed this in f839627Sep 20, 2016
This pull request was closed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@zxcpoiu@facebook-github-bot@christopherdro@philikon@dewe@mkonicek@javache