-
Notifications
You must be signed in to change notification settings - Fork 13
Adds a function to decode arraybuffers as their string representation. #15
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
Conversation
Refactor and cleanup Add prop tests for arraybuffer and base64 encoding Node compat for base64 Cleanup and working toward isomorphic interface between node and browser Need to determine where test failures are occurring. Better testing needed Better test feedback Delete faulty browser implementations Green tests, but still needing a browser implementation for base64 and arraybuffer manipulation return to old reliable fromStr function Working towards a unified interface for buffer handling Add test vector for b64 Remove test vector TYPE ERROR Cleanup preparing some front-end testing tools Remove old references to mocha Remove base64 encoding as it is growing in complexity. Moving base64 to its own pursuit package Remove stuff after change of mind Last of the cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the PR and sorry for the delay...
|
||
exports.fromIntArray = function(s) { | ||
return (new Uint8Array(s)).buffer; | ||
} | ||
}; | ||
|
||
exports.fromString = function(s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there any issue in the previous implementation besides the missing semicolon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I was just playing cleanup while I was in there.
src/Data/ArrayBuffer/ArrayBuffer.js
Outdated
return buf; | ||
}; | ||
|
||
exports.decodeToString = function(buffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return a Maybe
? What if you pass a 1 byte array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, yes. it absolutely should. An earlier revision did infact have a maybe, and I somehow forgot about that case as I went about simplifying.
src/Data/ArrayBuffer/ArrayBuffer.js
Outdated
}; | ||
const points = uintBuffer.reduce(reducer, new Array()); | ||
return points.join(""); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this one-liner at https://stackoverflow.com/questions/6965107/converting-between-strings-and-arraybuffers
function ab2str(buf) {
return String.fromCharCode.apply(null, new Uint16Array(buf));
}```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look again in a bit, but I did previously have that and the isomorphism test was failing. It was a week or more ago so I can't remember the reason for abdandoning that one-liner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted back to the one-liner, as it seems our isomorphism does indeed pass.
.gitignore
Outdated
@@ -3,4 +3,4 @@ output/ | |||
bower_components/ | |||
node_modules/ | |||
.psci | |||
.psci_modules/ | |||
.psci_modules/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should the newline be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnoticed editor mistake. Deleting it.
Merged, thanks! |
I spent a lot of time playing around with different base64 encoding things and adding support for browser testing, and eventually came to the conclusion that the transport-formats of an array buffer are fundamentally a different concern from dealing with array buffers themselves. I've squashed my work in progress here, which now just adds support for decoding an array buffer back into a string representation of it, using the uint16 view.
The base64 and transport stuff will move to its own pursuit package probably later this weekend or next weekend.