Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

io_utils: use Buffer methods instead of Blob/atob/btoa on Node.js #1135

Merged
merged 1 commit into from
Jul 5, 2018

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Jun 30, 2018

This should fix arrayBufferToBase64String and stringByteLength tests when running on Node.js.

Those tests are currently blacklisted with tfjs-node here:
https://github.com/tensorflow/tfjs-node/blob/master/src/run_tests.ts#L45-L46

Refs: tensorflow/tfjs#279


This change is Reviewable

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jun 30, 2018

I am not sure if this is the best path forward to implement those, but I wasn't able to find an existing mechanism for overriding those methods from tfjs-node backend, and this small change should be cleaner than implementing such a mechanism. Please correct me if I am wrong.

@ChALkeR ChALkeR changed the title io_utils: don't use atob/btoa on Node.js io_utils: use Buffer methods instead of Blob/atob/btoa on Node.js Jun 30, 2018
This should fix `arrayBufferToBase64String` and `stringByteLength`
tests when running on Node.js.
@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jul 4, 2018

/cc @nkreeger @dsmilkov

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Nice work! I think this approach of feature testing if the way to go. Sorry for the delay in review!

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 1 LGTMs obtained

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@dsmilkov dsmilkov merged commit 4eb73a2 into tensorflow:master Jul 5, 2018
@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jul 6, 2018

@ctemer That doesn't look related to this PR, does it?

@ctemer
Copy link

ctemer commented Jul 7, 2018

sorry misstyped

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants