-
Notifications
You must be signed in to change notification settings - Fork 575
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
Fix for issues mentioned in PR #3448 #3457
Conversation
🦋 Changeset detectedLatest commit: 03da6e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
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 |
/** | ||
* Client version to report to the usage reporting API | ||
*/ | ||
clientVersion?: StringFromRequestFn | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you are right but it is still in 0.x.x
version and according to semver docs it should not be big problem. However, it should be released as minor version. I will edit it. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just seperate the fix and the breaking changes into different PRs?
Because this breaking change will affect dependent packages too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separated "breaking changes" into a separate PR here #3458. Check it out please.
@@ -99,7 +98,7 @@ export function useApolloUsageReport(options: ApolloUsageReportOptions = {}): Pl | |||
} | |||
|
|||
let clientVersionFactory: StringFromRequestFn = req => | |||
req.headers.get('apollographql-client-version') || yoga.version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it nicer to have some information about the sender even if it is not dynamic because otherwise the information becomes anonymous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. 🙂 It will always be an anonymous call, even if we add some default value here. Additionally, if someone sends only "client name", then some unrelated version value should not be added here. As I mentioned above, it would be a mess. 🙂
It is normal behavior for Apollo Studio that the client name or version does not always appear in the trace and these metrics can be still properly filtered (as "unidentified client"). Even the official implementation of this plugin does not assign any default values. Check it out here.
✅ Benchmark Results
|
Description
Fixes issues mentioned in PR #3448
__YOGA_VERSION__
is sent to the traceclientName
andclientVersion
should not have any default values, because it is real identification of client (mobile app, web, ...). The client can send only thename
or both (name
andversion
) or even "nothing". Setting the default values in this case can make a lot of confusion and if the client does not send this information (name
orversion
) any default values should not be definitely set!clientName
andclientVersion
totype ApolloUsageReportOptions
as astring
(custom function is good idea 👍 )