-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add initial version of node-buffer-blob #1
Conversation
The node constructor uses a union type, accepting a list of blobs, a list of arraybuffers, a list of strings, .... |
Apart from this, I think this initial pr is ready for review. |
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.
Overall looks good. But there are still a few changes to be made.
Could you clarify why you didn't do FFI for blob.type
? Or implement Blob.from
for the following?
- Array TypedArray
- Array DataView
- Array Blob
package.json
Outdated
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "git+https://github.com/purescript-node/purescript-node-buffer-blob.git" |
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.
Could you drop the git+
part here?
return function() { | ||
return blob.text(); | ||
}; | ||
}; |
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.
There's no blob.type
FFI?
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.
Fixed
test/Node/Buffer/Blob.purs
Outdated
testFromStringWithOptions = do | ||
let | ||
expected = "{\"hello\":\"world\"}" | ||
blob = Blob.fromString expected (Just { "type": MediaTypes.applicationJSON, endings: Blob.Transparent }) | ||
actual <- Promise.toAffE $ Blob.text blob | ||
liftEffect $ assertEqual { actual, expected } |
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.
Shouldn't there be newlines in the expected string? Otherwise, the Blob.Transparent
part isn't being tested, right?
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.
Added newlines. Not sure though how this option should/could be tested
Also, I wonder if the singular versions of But if calling |
Yeah that makes sense. I changed everything to NonEmptyArrays. |
Could you clarify why you didn't implement
|
Oh I just missed these, will add them |
Ok I added |
I'm good with that. |
First version of node-buffer-blob