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

Standartize what open , k6/experimental/fs.open and k6/net/grpc.Client#load are relative to #3857

Open
mstoykov opened this issue Jul 18, 2024 · 0 comments
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes

Comments

@mstoykov
Copy link
Contributor

What?

Have open , k6/experimental/fs.open and k6/net/grpc.Client#load be relative to the same path for each execution.

Or at least specify and stick to one new norm among them and anything else related to loading files.

Why?

Consistency, ease of reasoning, also potentially debugging and using one in place of the other.

Current situation:

k6/experimental/fs.open and k6/net/grpc.Client#load are relative to the entry point of k6 - so either the folder of the argument to run or if is - (stdin) - the folder k6 is executed in.

I am pretty sure this was a mistake in gRPC (#2781). fs.open was more of "let's not make this more complicated especially given this is an experimental module".

open on the other hand is relative to the currently executed file, but I would argue by design it was supposed to be relative to the file it is called in. Similar to how require (previously buggy) is defined, and also how imports both static and dynamic usually work in ECMAScript. And how they are actually now implemented.

So for me this seems like a bug all around.

Examples of the behaviour:

If we have the following structure

/root/main.js
/path/A.JS
/path/B/B.js

main.js:

import "/path/A.JS"

A.js

import { openHelper, fsOpenHelper } from "./B/B.js";
import fs from "k6/experimental/fs";
open("./some/path/file.csv") // this is /path/some/path/file.csv - which seems pretty correct
fs.open("./some/path/file.csv") // this is /root/some/path/file.csv - which isn't the worst ... but still confusing
// same for grpc 
openHelper("./some/path/file.csv") // this is /path/some/path/file.csv - which is ... quite impossible to guess
fsOpenHelper("./some/path/file.csv") // this is /root/some/path/file.csv - at least this is consistent

B.js

import fs from "k6/experimental/fs";

export function openHelper(s) { open(s) }
export function fsOpenHelper(s) { fs.open(s) }

In this case the openHelper is arguably the "good" example for the current behaviour, but if you wanted to have helper that does something completely different but as part of it it opens

export function checkSomethingExists(id) {
    return open("./some/path/to/file.json")[id]
} 

That relative path will be relative to whatever calls checkSomethingExists as in the example above.

Proposal:

Change all of them to be relative to the file being called in - how imports and require work.

Given that the three currently have 3 different behaviours and only on of them is experimental - we will need to have at least a couple of releases with warnings about this.

I recommend we do the same as with require - test that the new implementation will not give a different result (which is possible in some cases) and only if it isn't - then emit a warning.

We can also recommend using #3856 if it is implemented at the time.

The existance of import.meta.resolve doesn't change the fact that the functions having different things they are relative to is a confusing behaviour.

Related issues and PRs

#2674
#2782
#3534

@mstoykov mstoykov added evaluation needed proposal needs to be validated or tested before fully implementing it in k6 breaking change for PRs that need to be mentioned in the breaking changes section of the release notes labels Jul 18, 2024
mstoykov added a commit that referenced this issue Oct 30, 2024
This code is similar to the one made for `require` but for `open`.

Part of #3857
Closes #3960
mstoykov added a commit that referenced this issue Nov 6, 2024
This code is similar to the one made for `require` but for `open`.

Part of #3857
Closes #3960

Co-authored-by: Joan López de la Franca Beltran <[email protected]>
@mstoykov mstoykov removed the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

No branches or pull requests

1 participant