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

Will close #269 at some point #370

Closed
wants to merge 4 commits into from

Conversation

ragnarlonn
Copy link

A PR for you to tear to pieces, o great maintainer

@ragnarlonn ragnarlonn requested a review from liclac November 27, 2017 13:30
@ragnarlonn
Copy link
Author

Note: the k6/encoding module should also be changed to recognize the TextOrBinary object as input parameter

@liclac
Copy link
Contributor

liclac commented Nov 28, 2017

TIL you can just change the parameters' type to []byte and it'll Just Work™. There's a weird edge case where passing []byte("hi") to a string parameter turns it into a string that says "104,105", I'm gonna open a goja issue about this.

@ragnarlonn
Copy link
Author

Which parameter are you talking about? And apologies if it is obvious - my brain is a bit woolly today (evil cold virus).

@liclac
Copy link
Contributor

liclac commented Nov 29, 2017

Any string parameter that should be able to accept bytes. I'm discussing a better solution with the goja maintainer, but this will work as a stopgap.

@ragnarlonn
Copy link
Author

@liclac So, I'm still a bit confused about exactly what you'd like me to change. Do you mean that the parameters to the crypto functions should be all []byte instead of goja.Value? That would mean you can't pass the binary data object you get back from open() directly to the crypto functions (or any other functions that want to consume binary data), but would have to do e.g. let dat = open("file.dat", "b"); hasher.update(dat.bytes()); instead of let dat = open("file.dat", "b"); hasher.update(dat); But maybe that is better?

@ragnarlonn
Copy link
Author

ICMP echo request -> @liclac

@liclac
Copy link
Contributor

liclac commented Dec 15, 2017

Oh, my response didn't send.

As dop251/goja#51 points out, we should either implement an ArrayBuffer-like (which we can PR to goja), or do the quick fix which is to pass around []byte objects, which will Just Work™.

@ragnarlonn
Copy link
Author

@liclac OK, I had some idea that it would be bad if we didn't know whether data was binary or not - that we might want other functions (like perhaps the crypto.* ones) to know this. If we just use a []byte parameter to pass it around we don't know if it is a legible string, or something that contains null bytes. But I can't really think of an example where it would give us trouble. I'll try just using byte slices everywhere and see what happens!

@ragnarlonn
Copy link
Author

@liclac OK, so here (below) is what happens when open() returns a byte slice and I read a text file and then try to print its contents on the console. I think this is what you meant with the earlier "weird edge case" comment, no?

I have a file that contains:

hello
world
blablabla

...and open()ing it and then console.log()ing the contents will print this:

INFO[0001] 104,101,108,108,111,10,119,111,114,108,100,10,98,108,97,98,108,97,98,108,97,10 

Am I understanding it correctly that you are of the opinion that we should just use []byte params anyway and break the ability to log to stdout text that was read from a file? That would mean the patch becomes very, very simple (3 lines of code changed, I think).

@liclac
Copy link
Contributor

liclac commented Dec 18, 2017

The easiest solution would probably be to let the console.* functions explicitly check if v.ExportType() is a []byte and cast to string in those cases, otherwise do v.String() like normal.

Copy link
Contributor

@liclac liclac left a comment

Choose a reason for hiding this comment

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

Remember to update http.Request to take []byte bodies too!

@@ -47,9 +48,16 @@ func (c Console) log(ctx *context.Context, level log.Level, msgobj goja.Value, a

fields := make(log.Fields)
for i, arg := range args {
fields[strconv.Itoa(i)] = arg.String()
if arg.ExportType() == reflect.TypeOf((*[]byte)(nil)).Elem() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can declare a global var typeBytes = reflect.TypeOf(...) to not have to recalculate this every time.

@robingustafsson
Copy link
Member

@ragnarlonn When will you be able to fix the merge conflict and move the type calculation that @liclac commented on in console.go? I want to get this merged so we can move #420 forward.

@robingustafsson
Copy link
Member

Closing this in favor of #524.

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.

3 participants