Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

zxcpoiu
Copy link
Contributor

@zxcpoiu zxcpoiu commented Apr 12, 2016 •
edited
Loading

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
Contributor

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

@facebook-github-bot facebook-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
Copy link
Contributor

christopherdro commented Apr 16, 2016 •
edited
Loading

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

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

zxcpoiu commented Apr 19, 2016

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

@zxcpoiu zxcpoiu mentioned this pull request Apr 19, 2016
Closed
@philikon
Copy link
Contributor

note: this PR now depends on #6961

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

philikon
philikon reviewed Apr 19, 2016
View reviewed changes
package.json
@@ -138,7 +138,7 @@
"babel-register": "^6.6.0",
"babel-types": "^6.6.4",
"babylon": "^6.6.4",
"base64-js": "^0.0.8",
"base64-js": "^1.0.4",
Copy link
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
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
Contributor

@zxcpoiu updated the pull request.

@zxcpoiu
Copy link
Contributor Author

zxcpoiu commented Apr 20, 2016

@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
Copy link

dewe commented Apr 20, 2016

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
Contributor Author

zxcpoiu commented Apr 20, 2016

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
Contributor

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

@ghost
Copy link

ghost commented May 5, 2016

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

@ghost ghost 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
@zxcpoiu
update base64-js version. follow up facebook#6327
9972880 
This is a follow up of PR facebook#6327
by @dewe and @jeremyong
as @mkonicek indicated this should split into a separate PR
@ghost ghost 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
Member

javache commented Aug 4, 2016

@facebook-github-bot import

@ghost
Copy link

ghost commented Aug 4, 2016

Thanks for importing.

@ghost ghost 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

ghost commented Aug 13, 2016

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?

@ghost ghost 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
Contributor

mkonicek commented Sep 9, 2016

Took a while to get back to this one :)

@facebook-github-bot import

@ghost
Copy link

ghost commented Sep 9, 2016

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

@ghost ghost 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
@ghost ghost closed this in f839627 Sep 20, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mkonicek mkonicek

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zxcpoiu @facebook-github-bot @christopherdro @philikon @dewe @mkonicek @javache