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

Support to init a ReadableStream from a io.Reader #3740

Merged
merged 7 commits into from
Jun 7, 2024

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented May 14, 2024

What?

It exposes a mechanism from streams (Go) package to initialize a new ReadableStream from a io.Reader:

  • NewReadableStreamForReader(vu modules.VU, reader io.Reader) *goja.Object

Why?

Because this would makes it possible to reuse the ReadableStream implementation added in v0.51.0 from Go code, so other pieces in k6 (or any k6 extension) that have a Go io.Reader and want to translate to a ReadableStream, usable from JS code, could it.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

N/A

@joanlopez joanlopez self-assigned this May 14, 2024
@oleiade
Copy link
Member

oleiade commented May 14, 2024

I think this is great 🚀 🚀 🚀

This is an idea I wanted to bring up here, but I'm not suggesting we should do it, consider it blocking, nor even arguing this is a good idea: I often find myself creating NewSomething() *goja.Object, and NewSomethingFrom(goja.Value) functions when working in extensions. A while ago, when working in WebCrypto, I experimented with the idea of leveraging Go generics to create From and To generic traits.

However, we ended up not needing generics at the time. I wonder if this idea would make sense in such a context, where for a type T, we can implement a trait To[T] *goja.Object and From(goja.Value) *T.

This is somewhat blurry in my head, but I keep thinking about it. Maybe this is something we could try in the future, although I'm not yet certain if it would have benefits beyond creating a contract for creating from and exporting as object given types (and I might be biased by Rust, where this would likely be the way to go in a lot of situations).

I don't know, but if you have thoughts, I'd love to hear them 😄

@joanlopez joanlopez marked this pull request as ready for review May 30, 2024 11:11
@joanlopez joanlopez requested a review from a team as a code owner May 30, 2024 11:11
@joanlopez joanlopez requested review from mstoykov, olegbespalov and oleiade and removed request for a team May 30, 2024 11:11
@joanlopez joanlopez added this to the v0.52.0 milestone Jun 4, 2024
olegbespalov
olegbespalov previously approved these changes Jun 5, 2024
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

It's not related to the PR, but it's worth using branches in the grafana/k6 repository. That way, if needed, another maintainer could pick up the work and finish it.

@joanlopez joanlopez requested a review from codebien June 6, 2024 11:02
@joanlopez joanlopez merged commit c5f57cc into grafana:master Jun 7, 2024
23 checks passed
})
}

func underlyingSourceForReader(vu modules.VU, reader io.Reader) *sobek.Object {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we miss a replacement here s/For/From/

"go.k6.io/k6/js/modulestest"
)

func TestNewReadableStreamForReader(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

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.

5 participants