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

Use k6 module system to load js customization #194

Closed
wants to merge 1 commit into from
Closed

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jul 1, 2024

What?

Move to using the k6 module system in order to load and run scripts

Why?

This is mostly needed as soon compiler.Compile will be gone. And even if not supporting ESM syntax will require actually having a full module system.

The change is quite big and there are some questionable things around getting the current working directory.

Checklist

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

Related PR(s)/Issue(s)

This is mostly needed as soon compiler.Compile will be gone. And even if
not supporting ESM syntax will require actually having a full module
system.

The change is quite big and there are some questionable things around
getting the current working directory.
@mstoykov mstoykov requested a review from a team as a code owner July 1, 2024 16:06
@mstoykov mstoykov requested review from pablochacin and removed request for a team July 1, 2024 16:06
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mstoykov mstoykov requested a review from szkiba July 1, 2024 16:06
@mstoykov
Copy link
Contributor Author

mstoykov commented Jul 1, 2024

Given that this isn't advertised at all I kind of would prefer if we just drop it and reintroduce it if there is enough interest later.

I have no idea if this js configuration adds anything that the JSON one doesn't , but if it doesn't it just seems like a very strange thing to support.

cc @dgzlopes @szkiba

@szkiba
Copy link
Collaborator

szkiba commented Jul 1, 2024

Given that this isn't advertised at all I kind of would prefer if we just drop it and reintroduce it if there is enough interest later.

I have no idea if this js configuration adds anything that the JSON one doesn't , but if it doesn't it just seems like a very strange thing to support.

cc @dgzlopes @szkiba

Agree, I recommend just removing this JS config feature.

@mstoykov
Copy link
Contributor Author

mstoykov commented Jul 2, 2024

closing in favor of #196

@mstoykov mstoykov closed this Jul 2, 2024
@mstoykov mstoykov deleted the moduleSystem branch July 2, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants