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

♻️ Simplify RUM assembly #1588

Merged
merged 4 commits into from
Jun 15, 2022

Conversation

amortemousque
Copy link
Contributor

@amortemousque amortemousque commented Jun 10, 2022

Motivation

RUM assembly has a quite convoluted logic to handle the various contexts. More precisely:

  • All context extends Context making us beleave they can contain any attribute ([x: string]: ContextValue) which is false.
  • It's hard to follow the overridden logic of context with the combine
  • It's hard to understand the service and version logic

Changes

  • Only RumContext now extends Context
  • Remove urlContext, viewContext, { action: { id: actionId } } from the combine and use simple assignment
  • Remove convoluted viewContext service and version logic and use simple assignment

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@amortemousque amortemousque marked this pull request as ready for review June 10, 2022 07:33
@amortemousque amortemousque requested a review from a team as a code owner June 10, 2022 07:33
@@ -181,7 +181,7 @@ export type RawRumEvent =
| RawRumLongTaskEvent
| RawRumActionEvent

export interface RumContext {
export interface RumContext extends Context {
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏ we need to extend Context here just to work around the Index signature for type 'string' is missing in type ... error, but RumContext is never actually used with an index, except when using combine.

Maybe we could remove the extends here, and cast the object as RumContext & Context where RumContext is used with combine. WDYT?

I know this is a bit confusing, and there is a long standing TS issue related to that.

Comment on lines 128 to 131
id: viewContext.view.id,
name: viewContext.view.name,
url: urlContext.view.url,
referrer: urlContext.view.referrer,
Copy link
Member

Choose a reason for hiding this comment

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

🥜 nitpick: ‏maybe remove the view extra-level from those contexts

@amortemousque amortemousque force-pushed the aymeric/simplify-view-context-assemble branch from 565e2c4 to b7923d7 Compare June 10, 2022 11:18
@amortemousque amortemousque force-pushed the aymeric/simplify-view-context-assemble branch from b7923d7 to 7721941 Compare June 10, 2022 12:50
@amortemousque amortemousque requested a review from a team as a code owner June 10, 2022 12:50
@amortemousque amortemousque merged commit dc6cf2a into main Jun 15, 2022
@amortemousque amortemousque deleted the aymeric/simplify-view-context-assemble branch June 15, 2022 07:49
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.

2 participants