Skip to content

feat: session id support for payloads - #32

Closed
EDsCODE wants to merge 29 commits into
masterfrom
session-id-support
Closed

EDsCODE wants to merge 29 commits into
masterfrom
session-id-support

Conversation

@EDsCODE

@EDsCODE EDsCODE commented Jul 25, 2022

Copy link
Copy Markdown
Contributor

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

Screenshots or screencasts (if UI/UX change)

Questions:

  • Does the docs need an update?
  • Are there any security concerns?
  • Do we need to update engineering / success?

Base automatically changed from feature-flag-support to master July 25, 2022 18:04
@EDsCODE
EDsCODE marked this pull request as ready for review July 25, 2022 18:40
Comment thread PostHog/Classes/PHGSession.m Outdated
@interface PHGSession ()

@property (nonatomic, copy) NSString *sessionId;
@property (nonatomic) NSTimeInterval sessionStartTimestamp;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i'm calling this a sessionLastTimestamp here instead to align with https://github.com/PostHog/posthog-js-lite/pull/4/files#diff-30dde81fd76ff386b3bde594b35a527f3bac4438ed7ef3183ed03ab756b4c562R34, shall we consolidate to that naming?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Works!

Comment thread PostHog/Classes/PHGSession.m Outdated

@interface PHGSession ()

@property (nonatomic, copy) NSString *sessionId;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

will these values persist across app restarts? 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or app backgrounding

@liyiy

liyiy commented Jul 26, 2022

Copy link
Copy Markdown
Contributor

I think we're also missing a resetSessionId method ?

@EDsCODE
EDsCODE requested a review from rcmarron August 29, 2022 16:06
@neilkakkar
neilkakkar requested review from benjackwhite and removed request for rcmarron October 13, 2022 14:25
@neilkakkar

Copy link
Copy Markdown
Contributor

Adding Ben just for sess-rec awareness

@benjackwhite

Copy link
Copy Markdown
Contributor

Happy to add a review but not sure it has much impact on sesh-rec world...?

@neilkakkar

Copy link
Copy Markdown
Contributor

D'oh, right, you wouldn't need this at all, since no session recordings on mobile 🤦

@neilkakkar neilkakkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, judging by the tests! (no swift-fu here 😅 )

@benjackwhite benjackwhite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice looks good!

@benjackwhite benjackwhite self-assigned this Jan 25, 2023
@marandaneto marandaneto mentioned this pull request Jan 19, 2024
@marandaneto

Copy link
Copy Markdown
Member

Closed in favor of #100

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.

5 participants