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

Update to [email protected] #3824

Merged
merged 1 commit into from
Jul 4, 2024
Merged

Update to [email protected] #3824

merged 1 commit into from
Jul 4, 2024

Conversation

joanlopez
Copy link
Contributor

What?

Updates to the latest version of xk6-websockets, which includes support for Blob.

See grafana/xk6-websockets#74 for further details.

Why?

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make 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)

Contributes to grafana/xk6-websockets#35

@joanlopez joanlopez added the documentation-needed A PR which will need a separate PR for documentation label Jul 3, 2024
@joanlopez joanlopez requested review from mstoykov and codebien July 3, 2024 11:01
@joanlopez joanlopez self-assigned this Jul 3, 2024
@joanlopez joanlopez requested a review from a team as a code owner July 3, 2024 11:01
@joanlopez
Copy link
Contributor Author

joanlopez commented Jul 3, 2024

Open question: should we tag it as breaking-change?

In my opinion, even if it's kinda expected/permitted/accepted (because the module is experimental), it's nice to list it as part of the breaking changes within Release Notes, so users can easily notice it, but I'm not sure what we usually do in such cases.

The breaking change is that now we return an error if websocket.binaryType isn't defined (there was a warning before).

cc/ @mstoykov @codebien

@mstoykov mstoykov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Jul 3, 2024
@codebien
Copy link
Contributor

codebien commented Jul 3, 2024

In my opinion, even if it's kinda expected/permitted/accepted (because the module is experimental)

It remains a breaking change, and it still makes sense to announce it. IMHO, the experimental concept is something on top. In the event we were strictly following semver, the fact that the module is experimental just allows us to not bump a major version.

@joanlopez
Copy link
Contributor Author

In my opinion, even if it's kinda expected/permitted/accepted (because the module is experimental)

It remains a breaking change, and it still makes sense to announce it. IMHO, the experimental concept is something on top. In the event we were strictly following semver, the fact that the module is experimental just allows us to not bump a major version.

I agree!

@joanlopez joanlopez merged commit 7ff3aa4 into master Jul 4, 2024
22 checks passed
@joanlopez joanlopez deleted the update-xk6-websockets branch July 4, 2024 09:19
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 documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants