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

Remove option to set clientName and clientVersion as a static string #3458

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kroupacz
Copy link
Contributor

@kroupacz kroupacz commented Nov 4, 2024

Description

This PR removes option to set clientName and clientVersion as a static string in ApolloUsageReportOptions, because from "Apollo metrics" point of view it does not make sense.

It's related to PR #3448

Copy link

changeset-bot bot commented Nov 4, 2024

🦋 Changeset detected

Latest commit: de1320a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-yoga/plugin-apollo-usage-report Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@EmrysMyrddin EmrysMyrddin left a comment

Choose a reason for hiding this comment

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

I think it's ok :-) But not sure if we want to release a breaking change already.

@ardatan Do you think we can merge ?

@ardatan
Copy link
Collaborator

ardatan commented Nov 5, 2024

@EmrysMyrddin This can be a future thing imo. We can make it an empty string by default but a breaking change would be too much.

@kroupacz
Copy link
Contributor Author

kroupacz commented Nov 5, 2024

As I already wrote in another thread, static values ​​for client name and version have no meaningful meaning. I can't imagine any use-case for that. And if someone ever really needs it, they can always use { clientName: () => 'my-static-client-name' }

@ardatan
Copy link
Collaborator

ardatan commented Nov 5, 2024

As I already wrote in another thread, static values ​​for client name and version have no meaningful meaning. I can't imagine any use-case for that. And if someone ever really needs it, they can always use { clientName: () => 'my-static-client-name' }

It is not about being useful or not. It is the API we will break. Even if it is useful or not, we can't just release this with a breaking change which will break the dependent packages so we will have to release a major for them.

@kroupacz
Copy link
Contributor Author

kroupacz commented Nov 5, 2024

From my point of view there should be no problem to do BC in this case. This plugin is still in 0.2.0 version.

https://semver.org/
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Additionally, this change would only be released as 0.3.0

@ardatan
Copy link
Collaborator

ardatan commented Nov 5, 2024

There are other dependent packages we have that will be affected. That's what I am worried about

@kroupacz
Copy link
Contributor Author

kroupacz commented Nov 5, 2024

I'm sorry, but I don't know which dependent packages you're talking about. I don't see any dependent package in this graphql-yoga repository.
I'm just saying that doing something like this in version 0.2.0 shouldn't be a problem, and if it is, there's something architecturally wrong somewhere.
Of course, the decision is not up to me. I only need to merge this #3457 related issue. 🙂

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