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

[fs] Add a fs.opensync function to the k6/experimental/fs module #3203

Closed
wants to merge 1 commit into from

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jul 17, 2023

What?

This Pull Request adds an fs.openSync function to the k6/experimental/fs module, as per #3143.

It allows to open a file as such:

const f = fs.openSync('myfile.txt')

As opposed to the current workaround necessary to go around the limitation of not being able to use await in the init context:

let file
(() => {
  await file = await fs.open('myfile.txt')
)();

Why?

This function acts as a workaround for the lack of support for the await keyword in the init context and is meant to go away as soon as we do. It aims at ensuring a good UX that's more intuitive than the existing workaround ☝🏻

We should document it as such and make it explicit that it won't be supported any further once we address the core issue and that the following syntax is possible:

let f = await fs.open('myfile.txt')

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

closes #3143

@oleiade oleiade added this to the v0.46.0 milestone Jul 17, 2023
@oleiade oleiade self-assigned this Jul 17, 2023
@oleiade oleiade marked this pull request as draft July 17, 2023 14:31
@oleiade oleiade changed the title Add an fs.opensync function to the k6/experimental/fs module [2/2] Add an fs.opensync function to the k6/experimental/fs module Jul 17, 2023
@oleiade oleiade changed the base branch from master to experimental/fs July 17, 2023 14:32
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2023

Codecov Report

❗ No coverage uploaded for pull request base (experimental/fs@9dd1a26). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 97c7be2 differs from pull request most recent head 937deff. Consider uploading reports for the commit 937deff to get more accurate results

Additional details and impacted files
@@                Coverage Diff                 @@
##             experimental/fs    #3203   +/-   ##
==================================================
  Coverage                   ?   73.17%           
==================================================
  Files                      ?      263           
  Lines                      ?    20010           
  Branches                   ?        0           
==================================================
  Hits                       ?    14642           
  Misses                     ?     4433           
  Partials                   ?      935           
Flag Coverage Δ
ubuntu 73.12% <0.00%> (?)
windows 73.01% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oleiade oleiade force-pushed the experimental/fs-opensync branch from a228a8a to 85738db Compare July 19, 2023 14:59
@oleiade oleiade force-pushed the experimental/fs branch 2 times, most recently from 66e56c9 to fb6aa54 Compare July 20, 2023 12:49
@oleiade oleiade force-pushed the experimental/fs-opensync branch from 85738db to 44cbe26 Compare July 20, 2023 12:54
@oleiade oleiade force-pushed the experimental/fs-opensync branch from 44cbe26 to 9d4c7f3 Compare July 20, 2023 13:03
@oleiade oleiade modified the milestones: v0.46.0, v0.47.0 Jul 25, 2023
@oleiade oleiade force-pushed the experimental/fs-opensync branch from 9d4c7f3 to 30747f8 Compare August 17, 2023 07:48
@oleiade oleiade force-pushed the experimental/fs branch 2 times, most recently from 6a1658b to 9dd1a26 Compare August 24, 2023 12:56
@oleiade oleiade force-pushed the experimental/fs-opensync branch from 30747f8 to 5445d8e Compare August 30, 2023 14:05
@oleiade oleiade force-pushed the experimental/fs-opensync branch from 5445d8e to 937deff Compare August 30, 2023 14:15
@oleiade oleiade changed the title [2/2] Add an fs.opensync function to the k6/experimental/fs module [fs] Add a fs.opensync function to the k6/experimental/fs module Aug 30, 2023
@oleiade oleiade marked this pull request as ready for review August 30, 2023 14:17
@github-actions github-actions bot requested a review from codebien August 30, 2023 14:17
@github-actions github-actions bot requested a review from olegbespalov August 30, 2023 14:17
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

We should document it as such and make it explicit that it won't be supported any further once we address the core issue and that the following syntax is possible

I'm not very happy with it. We are adding something that in a cycle could be removed.

If it is useful a lot of people will use it then removing it will make them a lot unhappy.
If it isn't useful then we could just not add.

I would wait for one more cycle before adding it to see the evolution around JS ES modules.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

I'm sharing concerns with @codebien regarding adding something temporary.

On the other side, it's an experimental module, and by our definition, the API isn't stable, so it's OK to have this for a few releases.

@oleiade
Copy link
Member Author

oleiade commented Sep 4, 2023

There seems to be a consensus that we should not add openSync if we know we won't need it in the foreseeable future.

I'm happy to oblige and will close this Pull Request as a result 🙇🏻

I would consider support for await in the init stage a must-have for taking the fs module out of experimental though, and will add that condition to the issue 👍🏻

@oleiade oleiade closed this Sep 4, 2023
@codebien codebien removed this from the v0.47.0 milestone Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants