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

k6-summary: update to v0.1.0 #113

Merged
merged 3 commits into from
Jan 4, 2024
Merged

k6-summary: update to v0.1.0 #113

merged 3 commits into from
Jan 4, 2024

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Dec 5, 2023

Description

Please fill in this template.

  • Use a meaningful title for the Pull Request. Include the name of the jslib added/modified.
  • Fill the description section of the Pull Request.
  • Test the change in your code, and ensure the npm run test command succeeds.
  • Run yarn run generate-homepage locally and verify the new homepage /lib/index.html file looks legit.

Select one of these and delete the others:

If adding a new jslib:

  • The Pull Request creates a /lib/{jslib_name} folder.
  • The Pull Request creates a /lib/{jslib_name}/{desired_version} folder.
  • The /lib/{jslib_name}/{desired_version}/index.js file containing the jslib's code bundle exists.
  • The Pull Request updates the supported.json file to contain an entry for the newly added jslib and its {desired_version}, as in the following example:
{
  "{jslib_name}": {
    // Available package versions
    "versions": [
      "{desired_version}"
    ],

    // (optional) Documentation's or repository's URL
    "docs-url": "{documentation_or_repository_url}",

    // (optional) As a default, the homepage will point to
    // a package's bundle's index.js. If your package's main
    // bundle name is different; set it here (see the AWS
    // package for instance).
    "bundle-filename": "{index.js}"
}
  • Tests have been added to /tests/basic.js and /tests/testSuite.js to ensure that the added jslib is importable and runnable by k6.

If publishing a new version of an existing jslib:

  • The Pull Request is labeled with the version bump label.
  • The Pull Request adds a /lib/{jslib_name}/{desired_version} folder.
  • The Pull Request adds a /lib/{jslib_name}/{desired_version}/index.js file containing the jslib's code bundle.
  • The Pull Request updates the supported.json file to contain an entry for the newly added jslib version, as in the following example:
{
  "my-lib": [
    "1.0.2",
    // Use semantic versioning
    "{desired_version}"
  ]
}
  • The Pull Request adds the relevant tests to the /tests/basic.js and /tests/testSuite.js files to ensure that the new version of the jslib is importable and runnable by k6.
  • Merge the Pull Request once it is green. PRs adding new jslib versions do not require to get a review to be merged 🚀.

@olegbespalov olegbespalov self-assigned this Dec 5, 2023
@olegbespalov olegbespalov marked this pull request as ready for review December 15, 2023 12:54
@olegbespalov olegbespalov requested review from a team as code owners December 15, 2023 12:54
@olegbespalov olegbespalov requested review from mstoykov, codebien, going-confetti and EdvinasDaugirdas and removed request for a team December 15, 2023 12:54
@olegbespalov
Copy link
Contributor Author

The build was produced by the CI and attached as release artifact https://github.com/grafana/k6-jslib-summary/releases/tag/v0.1.0

lib/k6-summary/README.md Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

I'm not too familiar with the codebase and I've noticed that some libs/versions are minified and some aren't, so I wanted to ask: do we have guidelines for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that was the case and created some kind of confusion, but we'd like to try set up most of the projects with minification (like in a PR grafana/k6-jslib-summary#8) and at some point automate updating of this repo code, by using the artifacts from the releases

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO minifying with adding the map file doesn't make much sense for k6.

k6 does by default download and load map files as to make developer experience better. As such all the savings from not minifying will be not existant.

I am pretty sure we minify others for:

  1. k6 did not have source map support
  2. this is what you just do with js ... 🤷

I guess we can make k6 stop doing this by default, but then the UX will be worse whenever there is an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, than I had a wrong assumption. How about just minify the scripts then?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO - it doesn't matter as much either way - which is why I approved this.

In the end on each test run k6 will download those files once.

If a user use k6 archiveand then runs the archive - only on the archive command they will be downloaded.

Which also means that in the cloud only on k6 cloud but ont on just rerunnign from the UI or if it is scheduled.

All in all I expect the amount of downloads is not significant to matter whether:

  1. we minify it.
  2. k6 has to download a map as well as 1
  3. we do not modify it k6 has to download slightly bigger file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then to go back to the original question, it seems to me that the guideline we would like to have is to not minify the scripts. And, the reason is so we can keep their projects simpler. Am I getting it right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codebien I believe that you don't. The main point of the discussion right now are source maps. Minifying and source maps are related, but the minifying doesn't require source maps.

@going-confetti going-confetti self-requested a review December 15, 2023 14:43
Copy link

@going-confetti going-confetti left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM!

With the caveat that minifying source doesn't really help k6 .. it makes it worse

#113 (comment)

@olegbespalov olegbespalov merged commit 7e648cc into main Jan 4, 2024
4 checks passed
@olegbespalov olegbespalov deleted the feat/summary-0.1.0 branch January 4, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants