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 ExportBytes allowing use of Uint8Array for binary requests #1287

Closed
wants to merge 9 commits into from

Conversation

caseylucas
Copy link

This PR is a starting point for discussion. (See #1020). I implemented support for using core-js Uint8Arrays as request body so that binary data can be sent in requests. The caveat is that the core-js Uint8Array implementation is slow (compared to golang byte slices). See BenchmarkExportBytes where I benchmarked a few different scenarios. If the JavaScript author just needs an array-like object that can be used for binary data, then I think k6 could proved a JS call to allocate a golang byte slice that is usable in JS (see example.ByteSlice func.) This is much faster than using Uint8Array. The ExportByte function (used by func (h *HTTP) Request()) as it stands now supports Uint8Array but also supports []byte as well.

@codecov-io
Copy link

codecov-io commented Dec 20, 2019

Codecov Report

Merging #1287 into master will decrease coverage by 0.05%.
The diff coverage is 65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1287      +/-   ##
==========================================
- Coverage   75.44%   75.38%   -0.06%     
==========================================
  Files         150      151       +1     
  Lines       10885    10924      +39     
==========================================
+ Hits         8212     8235      +23     
- Misses       2207     2216       +9     
- Partials      466      473       +7
Impacted Files Coverage Δ
js/modules/k6/http/request.go 82.23% <100%> (ø) ⬆️
js/common/export.go 64.1% <64.1%> (ø)
stats/cloud/collector.go 76.99% <0%> (-0.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b6b2a6...b50e284. Read the comment docs.

"github.com/dop251/goja"
)

func ExportBytes(rt *goja.Runtime, val goja.Value) interface {} {

Choose a reason for hiding this comment

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

exported function ExportBytes should have comment or be unexported (from golint)

for _, tt := range nativeTests {
t.Run(tt.name, func(t *testing.T) {
rt := goja.New()
val := rt.ToValue(tt.arg)

Choose a reason for hiding this comment

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

Using the variable on range scope tt in function literal (from scopelint)

t.Run(tt.name, func(t *testing.T) {
rt := goja.New()
val := rt.ToValue(tt.arg)
if got := ExportBytes(rt, val); !reflect.DeepEqual(got, tt.arg) {

Choose a reason for hiding this comment

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

Using the variable on range scope tt in function literal (from scopelint)

rt := goja.New()
val := rt.ToValue(tt.arg)
if got := ExportBytes(rt, val); !reflect.DeepEqual(got, tt.arg) {
t.Errorf("ExportBytes() = %v, want %v", got, tt.arg)

Choose a reason for hiding this comment

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

Using the variable on range scope tt in function literal (from scopelint)

// need core js for Uint8Array
_, err := rt.RunProgram(jslib.GetCoreJS())
assert.NoError(t, err)
_, err = rt.RunString(tt.jsCode)

Choose a reason for hiding this comment

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

Using the variable on range scope tt in function literal (from scopelint)

assert.NoError(t, err)
jsVal := rt.Get(tt.jsVarName)
got := ExportBytes(rt, jsVal)
if !reflect.DeepEqual(got, tt.want) {

Choose a reason for hiding this comment

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

Using the variable on range scope tt in function literal (from scopelint)

jsVal := rt.Get(tt.jsVarName)
got := ExportBytes(rt, jsVal)
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("ExportBytes() = %v, want %v", got, tt.want)

Choose a reason for hiding this comment

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

Using the variable on range scope tt in function literal (from scopelint)

// }
return buf
}

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

Suggested change

}

jsTests := []struct {
name string

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

t.Errorf("ExportBytes() = %v, want %v", got, tt.want)
}
})

Choose a reason for hiding this comment

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

unnecessary trailing newline (from whitespace)

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@mstoykov
Copy link
Contributor

mstoykov commented Nov 2, 2020

Hi @caseylucas , sorry for the extemely wait reply. We were in the middle of reevaluating and #1007 in order to finally be able to get it out and I meant to try this out and figure out if this can be used temporarely ... but never got to it.

At this point, goja has ArrayBuffer support and that is definetely way better than what you proposed. Again I still haven't actually gotten to trying it out with the k6 APIs and fixing them accordingly but at least from running some scripts that previously broke it seemed to work way better. Hopefully, in v0.30.0 we will get some time to test this and document how well (or not) it works.

Thanks again for this PR - it did lead to me looking into some other stuff at the time, and hopefully helped you and somebody else so it wasn't for nothing :)

@mstoykov mstoykov closed this Nov 2, 2020
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.

6 participants