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

Inner function breaks outer rest parameters #2131

Closed
efdknittlfrank opened this issue Sep 10, 2021 · 5 comments · Fixed by #2135
Closed

Inner function breaks outer rest parameters #2131

efdknittlfrank opened this issue Sep 10, 2021 · 5 comments · Fixed by #2135
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 high prio js-compat
Milestone

Comments

@efdknittlfrank
Copy link

efdknittlfrank commented Sep 10, 2021

Great work on k6 0.34.0 🍾! (I was already impatiently waiting for all the new goodies :))

I upgraded my environment and noticed that several of my tests are now failing. Analyzing the failed tests, I noticed a very peculiar and strange interaction of rest parameters with inner function declarations. The repro can be reduced to just a few lines, which I have attached at the bottom of this issue.

I assume this is more likely to be a bug in goja than in k6 and most probably was introduced by #2109. Since k6 is the only way I can test and run JS in goja at the moment, I'm posting the bug here (also, because this has worked before in k6 up until and including version 0.33.0. It stopped working with 0.34.0).

On a related note, I expected startup of tests to be faster with the new version (due to less work being done by babel), but in fact I was observing a small increase in startup times. At least this is what k6 is telling me when being run with the -v flag. On the other hand, the time is printed next to "Babel: transformed", so perhaps the transformation is slower now?

Environment

  • k6 version: 0.34.0
  • OS and version: Windows 10
  • Docker version and image, if applicable: loadimpact/k6:0.34.0

Expected Behavior

Rest parameters work, regardless of function body

Actual Behavior

Rest parameters stop working (empty array) as soon as the function contains an inner function (named, anonymous, or arrow) which references the rest param array.

Steps to Reproduce the Problem

function working(...rest) {
  console.log(rest.length);
}

function broken(...rest) {
  console.log(rest.length);
  () => rest;
}

function workaround(...rest) {
  console.log(rest.length);
  const alias = rest;
  () => alias;
}

export default function() {
  working(4,2); // 2
  broken(4,2); // 0
  workaround(4,2); // 2
}
@na--
Copy link
Member

na-- commented Sep 10, 2021

Thanks for creating this issue and sorry about the regression! You are quite likely right that it's because of the latest goja changes. 😞

We'll investigate and hopefully create a fix soon. It's also quite interesting that the Babel transformation times became longer when we disabled some of the plugins... 😕 Certainly deserves some investigation, it might be the case that the new goja version itself is slower in executing Babel, we have to check 🤷‍♂️

@na-- na-- added this to the v0.35.0 milestone Sep 10, 2021
@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Sep 10, 2021
mstoykov added a commit that referenced this issue Sep 12, 2021
@mstoykov
Copy link
Contributor

@efdknittlfrank can you please try #2135 and see if it fixes the issue in your original script?

@efdknittlfrank
Copy link
Author

efdknittlfrank commented Sep 13, 2021

@mstoykov thanks for the fast PR :) no need to be sorry, regressions happen. Thanks for your work on k6; it is a really awesome tool!

Maybe I'm blind, but where can I download a build? #2135 seems to have failing tests, maybe that's why it is skipping creating packages?

I'd assume if the examples from this bug report are fixed with the fixed goja version, my original script would also work. For what's it worth, here is the function from my script:

export function f(...cbs) {
  return () => Object.assign({}, ...callbacks.map(cb => cb()));
}

f(
  () => doSomething(),
  () => 1+2,
  () => {}
);

@mstoykov
Copy link
Contributor

@efdknittlfrank you need to build it yourself, the ci doesn't build releases for not ... releases ;) But here is a windows build
k6-pr2135.exe.zip

I tried your snippet (with some changes):

export function f(...cbs) {
          return () => Object.assign({}, ...cbs.map(cb => cb()));
}

console.log(JSON.stringify(f(
          () => "some",
          () => {return {"a": "123"}}
)()));

and am now getting {"0":"s","1":"o","2":"m","3":"e","a":"123"} with both v0.33.0 and the PR one. I still would prefer for you to run your whole script to see if it works in that case as well ;)

@efdknittlfrank
Copy link
Author

@mstoykov that was … fast! Thanks. First impression: it works! 🎉

@na-- na-- modified the milestones: v0.35.0, v0.35.1 Sep 13, 2021
mstoykov added a commit that referenced this issue Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 high prio js-compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants