Skip to content

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented May 9, 2022

This PR converts the Session class to a more FP style Session object with additional functions that replace the methods of the class.

API-wise, This PR makes the following changes:

  • new Session(context) => makeSession(context)
  • session.update(context) => updateSession(session, context)
  • session.close(status) => closeSession(session, status)

session.toJSON() is left untouched because this method is called when the Session object is serialized to the JSON object that's sent to the Sentry servers.

Additionally, this PR modifies the session-related interfaces. Interface Session now describes the session object while the SessionContext interface is used to make modifications to the session by calling updateSession.

Note that updateSession and closeSession mutate the session object that's passed in as a parameter. I originally wanted to return a new, mutated, object and leave the original one untouched instead of changing it. However this is unfortunately not possible (without bigger modifications) as BaseClient makes a modification to a session after it's already passed to sendSession to keep it from getting re-sent as a new session.

ref: https://getsentry.atlassian.net/browse/WEB-815

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 18.72 KB (-7.03% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 58.31 KB (-9.76% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.52 KB (-7.1% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 52.55 KB (-9.34% 🔽)
@sentry/browser - Webpack (gzipped + minified) 19.31 KB (-16.9% 🔽)
@sentry/browser - Webpack (minified) 61.42 KB (-24.84% 🔽)
@sentry/react - Webpack (gzipped + minified) 19.33 KB (-16.93% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 42.8 KB (-10.93% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 24.38 KB (-6.52% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 22.98 KB (-6.16% 🔽)

@Lms24 Lms24 added this to the 7.0.0 milestone May 9, 2022
@Lms24 Lms24 marked this pull request as ready for review May 9, 2022 15:51
@Lms24 Lms24 requested a review from AbhiPrasad May 9, 2022 15:52
@Lms24 Lms24 force-pushed the lms-ref-sessions branch from f8443eb to 6dc3e50 Compare May 11, 2022 11:16
@Lms24 Lms24 merged commit 656d7c4 into 7.x May 11, 2022
@Lms24 Lms24 deleted the lms-ref-sessions branch May 11, 2022 13:22
Lms24 added a commit that referenced this pull request May 11, 2022
fix after merging #5054 which left an unresolved conflict in Migration.MD that emerged while rebasing the PR onto the current 7.x branch.
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
convert the `Session` class to a more FP style `Session` object with additional functions that replace the methods of the class.

API-wise, this makes the following changes:
* `new Session(context)` => `makeSession(context)`
* `session.update(context)` => `updateSession(session, context)`
* `session.close(status)` => `closeSession(session, status)`

 `session.toJSON()` is left untouched because this method is called when the Session object is serialized to the JSON object that's sent to the Sentry servers.

Additionally, this modify the session-related interfaces. Interface `Session` now describes the session object while the `SessionContext` interface is used to make modifications to the session by calling `updateSession`.

Note that `updateSession` and `closeSession` mutate the session object that's passed in as a parameter. I originally wanted to return a new, mutated, object and leave the original one untouched instead of changing it. However this is unfortunately not possible (without bigger modifications) as `BaseClient` makes a modification to a session after it's already passed to `sendSession` to keep it from getting re-sent as a new session.
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
fix after merging #5054 which left an unresolved conflict in Migration.MD that emerged while rebasing the PR onto the current 7.x branch.
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.

4 participants