refactor(types,shared,elements): Assign telemetry property to main Clerk interface
#3329
Conversation
🦋 Changeset detectedLatest commit: 4adcdd5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 |
LauraBeatris
changed the title
TelemetryCollectortelemetry property to main Clerk interface
LauraBeatris
force-pushed
the
add-telemetry-collector-type-interface
branch
from
6e21347
to
1447a67
Compare
LauraBeatris
force-pushed
the
add-telemetry-collector-type-interface
branch
from
1447a67
to
b0fb616
Compare
| @@ -151,7 +151,7 @@ export class Clerk implements ClerkInterface { | |||
| public organization: OrganizationResource | null | undefined; | |||
| public user: UserResource | null | undefined; | |||
| public __internal_country?: string | null; | |||
| public telemetry?: TelemetryCollector; | |||
| public telemetry: TelemetryCollector | undefined; | |||
There was a problem hiding this comment.
Updated to | undefined instead of |, otherwise it leads to a build error: Property 'telemetry' is optional in type 'Clerk' but required in type 'Clerk'.
I think it's also good to keep it consistent with the other attributes in here.
packages/types/src/telemetry.ts
Outdated
| export type TelemetryEvent = { | ||
| event: string; | ||
| /** | ||
| * publishableKey | ||
| */ | ||
| pk?: string; | ||
| /** | ||
| * secretKey | ||
| */ | ||
| sk?: string; | ||
| /** | ||
| * instanceType | ||
| */ | ||
| it: InstanceType; | ||
| /** | ||
| * clerkVersion | ||
| */ | ||
| cv: string; | ||
| /** | ||
| * SDK | ||
| */ | ||
| sdk?: string; | ||
| /** | ||
| * SDK Version | ||
| */ | ||
| sdkv?: string; | ||
| payload: Record<string, string | number | boolean>; | ||
| }; | ||
|
|
||
| export type TelemetryEventRaw<Payload = TelemetryEvent['payload']> = { | ||
| event: TelemetryEvent['event']; | ||
| eventSamplingRate?: number; | ||
| payload: Payload; | ||
| }; |
There was a problem hiding this comment.
Now that we are exposing these as public API i feel that we need to do the following
Rename TelemetryEventRaw to TelemetryEvent
Rename TelemetryEvent to TelemetryRecordedEvent or TelemetryStoredEvent
There was a problem hiding this comment.
I agree that Raw seems a bit generic here 🤔 - @BRKalow do you have other suggestions on that naming?
There was a problem hiding this comment.
🤔 I wouldn't really consider these part of the public API, it's an internal type that we're forced to expose due to how our packages are structured...
Raw feels accurate because it's the "raw" event that we're collecting before transforming it to send to our backend. TelemetryRecordedEvent is fine if we want to rename that.
We could also put a JSDoc with @internal to make it clear these aren't meant for public usage:
/**
* @internal
*/
export type TelemetryEventRaw = ...There was a problem hiding this comment.
Raw feels accurate because it's the "raw" event that we're collecting before transforming it to send to our backend. TelemetryRecordedEvent is fine if we want to rename that.
@BRKalow thanks for the context! I'm going to move forward with adding @internal JSDocs to those interfaces.
|
@LauraBeatris With this change, this piece of code can be updated as well |
LauraBeatris
changed the title
telemetry property to main Clerk interfacetelemetry property to main Clerk interface
LauraBeatris
force-pushed
the
add-telemetry-collector-type-interface
branch
from
5f62e54
to
c86ab3e
Compare
@panteliselef I've just removed the assertion & It seems that it was only added to override the |
Description
Unblocks SDK-1672
Assigns
telemetryproperty to main Clerk instance to access it within React hooks fromsharedpackage, like so:Checklist
npm testruns as expected.npm run buildruns as expected.Type of change