-
Notifications
You must be signed in to change notification settings - Fork 6
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 support for Blob #74
Conversation
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.
No specific feedback from my side, what Mihails said + linter fixes sounds like to plan to address 👍
nit: There are few places where we just throw an error like common.Throw(rt, err)
, I'd suggest add some context by wrapping the error
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.
Looks good to me!
Please also update README.md since it contains some Blob references that become misleading after merging this 👍
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 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.
LGTM in general but is missing the fairly important part of using Blob to send data.
So I will be requesting that as well on top of some nitpicks and the dependency updates :)
} | ||
|
||
// toByteSlice converts a slice of numbers to a slice of bytes. | ||
func toByteSlice(data interface{}) []byte { |
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'm open for suggestions, although none of the ones I've seen so far really convinces me. If you prefer any other, just tell me.
Hey @mstoykov, On top of nitpicks, I've added a bunch of changes:
So, with all that, I think we're in a quite decent position (after your review), to ship this support, as I think it's quite compliant. Thanks! 🙇🏻 |
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.
LGTM in general, but the isBlob part is actually blocking.
I will be glad to have more tests especially if we can get some suite working, but we can have this after we merge this.
Fixed, thanks to your suggestion! 🙇🏻
To be honest, after my experience with Streams API, and considering that experience wasn't smooth for other extensions (see grafana/xk6-webcrypto#81), I'd prefer to wait until we either make this extension part of k6-core or we make k6 surface for extensions that want to test against WPTs more reusable. In fact, arguably that could even be considered a requirement if we eventually support dynamic extension (more chances to keep extension isolated). Also, considering v1.0 guarantees, we can try to think about something reusable that isn't strictly part of k6 core surface/guarantees. In any case, I'd prefer to move on and postpone that discussion. I can create an equivalent of grafana/xk6-webcrypto#81 for this extension, if you want. All that said, as I mentioned here, I did code this implementation within the k6 code base locally, so we don't have checks in place for future chances, but technically this implementation is already partly compliant with WPTs. It's something. |
What?
It adds support for Blob, as described in #35
Why?
In order to stabilize the API before considering the extension non-experimental.
Checklist
make lint
) and all checks pass.make test
) and all tests pass.Related PR(s)/Issue(s)
Closes #35