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

feat(types): Allow user.id to be a number #8330

Merged
merged 2 commits into from Jul 11, 2023
Merged

feat(types): Allow user.id to be a number #8330

merged 2 commits into from Jul 11, 2023

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Jun 14, 2023

No description provided.

@HazAT HazAT self-assigned this Jun 14, 2023
Copy link
Member Author

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

Update session did as well

@@ -6,7 +6,7 @@ export interface RequestSession {

export interface Session {
sid: string;
did?: string;
did?: string | number;
Copy link
Member

Choose a reason for hiding this comment

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

https://develop.sentry.dev/sdk/sessions/#session-update-payload

did is typed as string in Relay. If we wanna allow number here we have to stringify it before sending.

Copy link
Member

Choose a reason for hiding this comment

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

did should be string - we should only be changing user.id

Copy link
Member

Choose a reason for hiding this comment

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

We can change what we allow here but we should only send the stringified version. Which we already do - I think Daniel found that out.

@@ -1,7 +1,7 @@
/** JSDoc */
export interface User {
[key: string]: any;
id?: string;
id?: string | number;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like relay theoretically allows numbers here:

https://github.com/getsentry/relay/blob/4a7c73a1e3e00215929b4b76644043771517c08c/relay-general/src/protocol/user.rs#L49

LenientString is like python's str(). We should still double check this.

Copy link
Member

Choose a reason for hiding this comment

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

LenientString allows any json type

@lforst lforst changed the title Allow user.id to be number as well feat(types): Allow user.id to be number Jun 14, 2023
@lforst lforst changed the title feat(types): Allow user.id to be number feat(types): Allow user.id to be a number Jun 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@HazAT HazAT merged commit 130e4a3 into develop Jul 11, 2023
62 checks passed
@HazAT HazAT deleted the ref/user-id-number branch July 11, 2023 16:59
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.

None yet

4 participants