Skip to content
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

Merged
merged 13 commits into from
Jun 9, 2022
Merged

Conversation

sigma-andex
Copy link
Collaborator

First version of node-buffer-blob

@sigma-andex
Copy link
Collaborator Author

The node constructor uses a union type, accepting a list of blobs, a list of arraybuffers, a list of strings, ....
I have created constructor functions fromArrays, fromBlobs for the moment. Not sure if there is a better way to do it without depending on something like untagged-union..

@sigma-andex
Copy link
Collaborator Author

Apart from this, I think this initial pr is ready for review.

@sigma-andex
Copy link
Collaborator Author

🏓 @JordanMartinez

Copy link

@JordanMartinez JordanMartinez left a 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"

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();
};
};

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 42 to 47
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 }

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?

Copy link
Collaborator Author

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

@JordanMartinez
Copy link

Also, I wonder if the singular versions of Blob.from (e.g. fromString) should be dropped. It's not too hard to go from fromString "foo" to fromStrings <<< Array.singleton or just fromStrings ["foo"]. Then there's less FFI to write.

But if calling blob.from([]) is problematic, then perhaps it should be NonEmptyArray a where a is one of those 5 types that Blob.from accepts.

@sigma-andex
Copy link
Collaborator Author

Also, I wonder if the singular versions of Blob.from (e.g. fromString) should be dropped. It's not too hard to go from fromString "foo" to fromStrings <<< Array.singleton or just fromStrings ["foo"]. Then there's less FFI to write.

But if calling blob.from([]) is problematic, then perhaps it should be NonEmptyArray a where a is one of those 5 types that Blob.from accepts.

Yeah that makes sense. I changed everything to NonEmptyArrays.

@JordanMartinez
Copy link

Could you clarify why you didn't implement Blob.from for the following types?

NonEmptyArray TypedArray
NonEmptyArray DataView
NonEmptyArray Blob

@sigma-andex
Copy link
Collaborator Author

Could you clarify why you didn't implement Blob.from for the following types?

Oh I just missed these, will add them

@sigma-andex
Copy link
Collaborator Author

Ok I added fromBlobs and fromDataView. I haven't yet figured out how the TypedArray works and at this point I would prefer to have that addressed in a subsequent PR if someone requires it.

@JordanMartinez
Copy link

Ok I added fromBlobs and fromDataView. I haven't yet figured out how the TypedArray works and at this point I would prefer to have that addressed in a subsequent PR if someone requires it.

I'm good with that.

@JordanMartinez JordanMartinez merged commit a2c6109 into main Jun 9, 2022
@JordanMartinez JordanMartinez deleted the add-node-buffer-blob branch June 9, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants