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

JavaScript spread operator throws error with params object #824

Closed
ericksoen opened this issue Oct 29, 2018 · 16 comments · Fixed by #3456
Closed

JavaScript spread operator throws error with params object #824

ericksoen opened this issue Oct 29, 2018 · 16 comments · Fixed by #3456

Comments

@ericksoen
Copy link

It would be nice to have the ability to configure a default params object with some common behaviors (authorization, tags, etc.) that request methods can overwrite using the spread operator as necessary.

For example, this is valid syntax in ES6.

let params = {
    headers: {
        "Content-Type": "Application/JSON",
    },
    timeout: 2000
};

let override = { ...params, timeout: 5000};

console.log(override);
// { headers: { 'Content-Type': 'Application/JSON' }, timeout: 5000 }

When I attempt the same behavior in a K6 test script, I get this error message:
image

Code sample to reproduce: google-search.js

import { check } from 'k6';
import http from 'k6/http';

export default function () {
    let params = {
        headers: {
            "Content-Type": "Application/JSON",
        },
        timeout: 5000
    };
    let requests = {
        "googleSearchWithQueryString": {
            method: "GET",
            url: `https://www.google.com/search?q=k6`,
            params: { ...params, timeout: 1000}
        },        
    };

    let responses = http.batch(requests);

    check(responses["googleSearchWithQueryString"], {
        "add concept code result was 200": res => res.status === 200
    });

}
@mstoykov
Copy link
Contributor

mstoykov commented Oct 30, 2018

k6 uses goja as JS VM and goja is only 5.1 ES compliant :(. We also use Babel to transform ES6 to ES5.1 JS but that obviously doesn't work always. There are two plugins for this but I will have to test them before I can tell you if it's going to work.
Can you try Object.assign in the meantime?

@ericksoen
Copy link
Author

ericksoen commented Oct 30, 2018

Thanks. That makes sense as a viable workaround. The fix ends up looking like this

import { check } from 'k6';
import http from 'k6/http';

export default function () {
    let params = {
        headers: {
            "Content-Type": "Application/JSON",
        },
        timeout: 5000
    };
    let requests = {
        "googleSearchWithQueryString": {
            method: "GET",
            url: `https://www.google.com/search?q=k6`,
            params: Object.assign({}, params, {timeout: 1000})
        },        
    };

    let responses = http.batch(requests);

    check(responses["googleSearchWithQueryString"], {
        "add concept code result was 200": res => res.status === 200
    });

}

// { headers: Object { Content-Type: "Application/JSON" }, timeout: 1000 }

@mstoykov
Copy link
Contributor

Nice! Going to keep this open for when we decide to actually have the spread operator

@sniku
Copy link
Collaborator

sniku commented Jul 11, 2019

I just wanted to add my +1 to this issue because I've run into the same problem on my end. The spread operator is very convenient for having default tags or headers and just extending them on specific requests as I tried here:
07_11_2019__11:34:17

@drfiresign
Copy link

More fuel to the fire here. Spread operators ftw! Also thank you for sharing the Object.assign() workaround. I was not aware of that myself.

@Spiderpig86
Copy link

+1, would really like to see ES6 support.

@na--
Copy link
Member

na-- commented Jul 8, 2020

To be fair, we have ES6 support. k6 supports the spread operator in function calls and array expansion, it just doesn't work for object literals, which is something new from ECMAScript 2018.

export default function () {
    console.log(...["this", "works"]);

    console.log(...[...["this", "also"], "works"]);

    // This doesn't work though
    //let a = { ...{ foo: "bar" }, test: "mest" }
}

@aboodman
Copy link

aboodman commented Jun 7, 2021

Hey k6 team, love your product, but this has bitten me a few times. It's hard to keep track of what's in k6 vs what's in browsers. It would be amazing to keep js support close to what's in modern browser (perhaps by just using v8?).

@mstoykov
Copy link
Contributor

mstoykov commented Jun 8, 2021

Hey k6 team, love your product,

Glad to hear that 🤗

perhaps by just using v8

As I have heard this a couple of times ... let's discuss what under understatement the just using v8 is ;) :
tl;dr "just using v8" is more or less "rewrite k6".

Let's start with that k6 is written in go and for the many great things go does, one thing that it isn't really great is ... calling non-go code (generally C or at least ... something that looks like C). In all cases calling into C in go is expensive as(overly simplified):

  1. Go doesn't actually have the same stack as it is expected by C programs, which means that it needs to ... make the stack look somewhat the same and move to it ... which is expensive ... especially as it probably wants to go back to a "go" stack after that.
  2. Go doesn't have a way to stop the C code execution in the middle in order for the thread to run something else, which is possible with a goroutine (albeit only at specific places(many asterisks here) - this leads to creating more OS threads. I personally got bitten by this as this is the same behavior if you read/write to an FS(as you need to call into the kernel) where a webserver was using 1500+ OS threads ... which wasn't great for performance ;)

For the record depending on which source you like consult, calling a go function vs calling cgo function has a difference between 10 and 100+ times. Which arguably isn't that bad ... if you are calling once, but k6 runs hundreds if not thousands of VMs all of which can effectively require full-blown OS threads and I would argue most of the js code executed is ... not worthy of the JIT in v8.

Some fairly (IMO) common looking k6 code:

var resp = http.get("someurl")
if (resp.json().something.or.other == 12) {
   var resp2 = http.get("another URL");;
   check(resp2, {
      "status 200": (r)=> r.status == 200,
      "has correct body": (r)=>r.json().something.or.other.second.time == "cool value",
   });
}

In this code we mostly just move between js and go and transform (or hopefully don't) values from js ones to go ones. Our current VM - goja, written in golang, does a great job of not actually having to transform from golang types to js types and back with us having to do mostly ... nothing, which means that we can ... add features to k6 instead of fighting the JS VM.

I also just tried to install https://github.com/augustoroman/v8 locally on my fedora and .... no luck, the v8 dependency install script requires ubuntu ... so I will likely manage to make it work with an additional docker in the mix but to be honest given that the last commit is from 2 years ago and they support v8 version 6.7. (object spread was actually added in v6.0 so at least that is in there) I don't think I will go through the hoops to get it to work. But the thing is that there isn't a single up-to-date v8 binding project for go as far as I can find and even if there is one ...

We lose cross-compatibility and make the whole release, build and test process a lot more complicated. K6 is currently being officially released for windows, mac os and Linux with later two supporting both amd64 and arm64 .... this means we need to somehow compile v8(or get it precompiled) for all of those on each release ... and run tests against this. Given my previous adventures in using cgo ... it does add quite a bit to the build time ;). Also there is very little OS specific code, most of which has to do with detecting errors, which arguably means that we support all OSs that k6 compiles to and all anyone needs to do is run go install go.k6.io/k6@latest and ... it should work 🎉 . You can look at the repo linked for the steps to get v8(and follow them to get the feel of the difference).

Also of note is that while you might think adding to the build time is not that big of a deal this makes the development slower. Currently k6 builds more or less instantly for small changes and running parts of the test locally is also super fast (under second to a few seconds for some of the bigger packages). Adding any amount of c will explode this build times considerably enough and likely will make the built in race detector not ... able to handle races with the C code(note: v8 is actually in C++ which doesn't help)

Also of importance is that while adding v8 gives us access to a lot of stuff ... we still actually need to integrate them in k6, which arguably will be a lot of work, like for example event loops will require quite a considerable rewrite of how we deal with concurrency in k6. But yeah ... purely syntax changes will just work ;)

One final nail is that currently the js package ... mostly the bindings for js in k6 and relative stuff is around 20k lines and the whole k6 (without the vendored dependencies) is 56k lines so quite a lot of the code is also very js dependant. (Arguably a lot of those are actually tests, but they will still likely need to be updated quite considerably :( )

Given all of the above (I probably could continue, but I see no point listing more stuff) "just using v8" is more or less synonymous with "rewrite k6" ... possibly in not go so we don't suffer all the go problems with cgo, without probably being able to use it lightweight concurrency.

Finally... just so I am not so negative - there is a branch adding destructuring to goja, which also works on the spread operator(it's kind of related ;) ), so if you want you can try and help there as it seemed to mostly work, but still needs to handle even more cases ;)

@aboodman
Copy link

aboodman commented Jun 8, 2021

Thanks for writing back and apologies for the lazy use of the word "just". I get it.

I hope that goja keeps relatively up to date with modern browsers since I enjoy using k6 in large part because I get to reuse skills from web development.

@mstoykov
Copy link
Contributor

mstoykov commented Sep 8, 2021

This is now fixed in #2109

@mstoykov mstoykov closed this as completed Sep 8, 2021
@mstoykov
Copy link
Contributor

mstoykov commented Sep 9, 2021

Unfortunately, it turns out that while it's fixed in goja, babel, which will be called on code that goja doesn't support such as import/export does not support it and even though it no longer needs to transpile it, it still needs to parse it so it can do w/e other transformations it needs and than spill it back out.

I am reopening this although I don't know of anything, in particular, we can do apart from removing babel or updating it 🤷

@mstoykov mstoykov reopened this Sep 9, 2021
@max-pfeiffer
Copy link

Plus one here as well. I wanted to use the spread operator for request tagging and it is still not working in the latest K6 version. While looking into it, I stumbled accross this already reported issue.

@nishant-shah-social
Copy link

my +1 as well for this issue. Its convenient to use spread operator. Yes.. object.assign is a workaround but then you need to follow 2 different conventions in your dev code and test code which is not too convenient

@DaleMckeown
Copy link

Still no spread!?!

@xepozz
Copy link

xepozz commented Feb 5, 2024

up there

@mstoykov mstoykov removed the triage label Feb 22, 2024
@mstoykov mstoykov linked a pull request Jul 8, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.