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

Fix require to be specification compliant in respect to what it resolves relative to #3534

Closed
mstoykov opened this issue Jan 11, 2024 · 5 comments · Fixed by #3456
Closed

Comments

@mstoykov
Copy link
Contributor

mstoykov commented Jan 11, 2024

Brief summary

k6's require has always resolved specifiers relative to the current "root of execution" module/script instead of to relative to the script/module the file is in.

An example below will help illustrate what "root of execution" is.

This behavior has been around forever and is against the specification for commonJS and is not how dynamic import works or to be honest anything else I have come around.

The same issue is true for open, but:

  1. this is issues is require specific to limit it scope
  2. open is not based on any specification

For the rest of the issue I might add "Also relevant to open" as reminder for this, but open is not what we discuss here.

This is extracted from #2674 within a more actionable issue

k6 version

all

OS

all

Docker version and image (if applicable)

No response

Steps to reproduce the problem

Have 3 files:
main.js

const s = require("./A/a.js")
if (s() != 5) {
	throw "Bad"
}
module.exports.default = () =>{} // just to not error

/A/a.js:

module.exports =  function () {
  return require("./b.js");
}

/A/b.js

module.exports = 5

This should not throw an exception - but it does. As at the time s() is executed the "root of the execution" is main.js not /A/a.js and consequently k6 resolves ./b.js based on that and then loading it from disk doesn't find it leading to

ERRO[0000] GoError: The moduleSpecifier "./b.js" couldn't be found on local disk. Make sure that you've specified the right path to the file. If you're running k6 using the Docker image make sure you have mounted the local directory (-v /local/path/:/inside/docker/path) containing your script and modules so that they're accessible by k6 from inside of the container, see https://k6.io/docs/using-k6/modules#using-local-modules-with-docker.
        at go.k6.io/k6/js.(*requireImpl).require-fm (native)
        at file:///home/mstoykov/work/k6io/k6/path-based-tests-playground/issue-example/A/a.js:3:16(3)
        at file:///home/mstoykov/work/k6io/k6/path-based-tests-playground/issue-example/main.js:3:6(7)  hint="script exception"

Expected behaviour

Script runs as expected and there is no exception.

Actual behaviour

ERRO[0000] GoError: The moduleSpecifier "./b.js" couldn't be found on local disk. Make sure that you've specified the right path to the file. If you're running k6 using the Docker image make sure you have mounted the local directory (-v /local/path/:/inside/docker/path) containing your script and modules so that they're accessible by k6 from inside of the container, see https://k6.io/docs/using-k6/modules#using-local-modules-with-docker.
        at go.k6.io/k6/js.(*requireImpl).require-fm (native)
        at file:///home/mstoykov/work/k6io/k6/path-based-tests-playground/issue-example/A/a.js:3:16(3)
        at file:///home/mstoykov/work/k6io/k6/path-based-tests-playground/issue-example/main.js:3:6(7)  hint="script exception"
@mstoykov
Copy link
Contributor Author

Proposed fix:

  1. Start warning users on cases where this happens
  2. Switch behavior, keep warning users if there are discrepancies
  3. Stop warning users.

Open questions:

  1. How long should the time periods between those be
  2. ?

Likelihood of someone actually hitting this?

IMO it is really low. All cases where babel (or any other tool) will transpile ESM code to commonJS will not hit at all.
Those arguably are most of our users.
Handwritten code with require is very rare - I have never seen someone in the k6 community doing it apart from examples such as this one. And then they will need to basically have a require in a function, export it and call it from a different file.

This also can potentially be breaking some third-party library, although it is also a thing I have never noticed.

As such I expect we will not hear from users at all on this. Unlike for open where some of the above have been noticed.

mstoykov added a commit that referenced this issue Apr 22, 2024
part of #3534

Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com>
@Gikkman
Copy link

Gikkman commented May 28, 2024

I was referenced here from a warning in my CLI, but it seems to not really be related?

Save the following as script.js

import http from 'k6/http';
import { URL } from "https://jslib.k6.io/url/1.0.0/index.js"
import { sleep } from 'k6';

export const options = {
    vus: 1,
    duration: '10s',
};

export default function () {
    const appUrl = new URL('http://example.com');
    http.get(appUrl.toString())
    sleep(5);
}

Then run it through docker:

$ docker run --rm --net=host -i grafana/k6 run - <script.js  

time="2024-05-28T11:57:33Z" level=warning msg="The \"wrong\" path (\"file:///\") and the path actually used by k6 (\"file:///home/k6/\") to resolve \"https://jslib.k6.io/url/1.0.0/index.js\" are different. Please report to issue https://github.com/grafana/k6/issues/3534. This will not currently break your script but *WILL* in the future, please report this!!!"

@kleijnweb
Copy link

kleijnweb commented May 29, 2024

I'm dynamically including modules that contain the functions for scenarios' "exec" and also running into this.

It's not great having my console absolutely trashed with warnings. Any way to shut this up?

Edit: I was able to work around by delegating the require to the execution root, passing the path down.

@mstoykov
Copy link
Contributor Author

@Gikkman Sorry about this and thanks for reporting this.

The problem doesn't really impact the correctness of your script nor will it ever. But it caught a case where it will report this wrongly and I have fix, but no test for it. So probably will make a PR on monday.

@kleijnweb without a script to reproduce this I can't tell you if you will actually be affected by this change or like @Gikkman this was a red herring.

But if you are truly affected as the message suggests your script will likely break in few releases when we fix how require and imports resolve relative specifiers.

@kleijnweb
Copy link

I delegated the require to be done in the "execution root". So either way the path will be correct.

@olegbespalov olegbespalov removed their assignment Jun 20, 2024
mstoykov added a commit that referenced this issue Jun 27, 2024
mstoykov added a commit that referenced this issue Jul 2, 2024
mstoykov added a commit that referenced this issue Jul 5, 2024
mstoykov added a commit that referenced this issue Jul 5, 2024
mstoykov added a commit that referenced this issue Jul 8, 2024
@mstoykov mstoykov linked a pull request Jul 8, 2024 that will close this issue
5 tasks
@mstoykov mstoykov mentioned this issue Jul 8, 2024
5 tasks
mstoykov added a commit that referenced this issue Jul 9, 2024
mstoykov added a commit that referenced this issue Jul 9, 2024
mstoykov added a commit that referenced this issue Jul 9, 2024
mstoykov added a commit that referenced this issue Jul 12, 2024
mstoykov added a commit that referenced this issue Jul 15, 2024
mstoykov added a commit that referenced this issue Jul 15, 2024
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.

4 participants