♻️ [RUM-8767] Use assembly hooks for global, user and account context #3457
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3457 +/- ##
==========================================
- Coverage 91.98% 91.98% -0.01%
==========================================
Files 310 309 -1
Lines 8026 8031 +5
Branches 1820 1821 +1
==========================================
+ Hits 7383 7387 +4
- Misses 643 644 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| accountContext = startAccountContext(customerDataTrackerManager, mockRumConfiguration()) | ||
| }) | ||
|
|
||
| it('should get account', () => { | ||
| accountContext.setContext({ id: '123' }) | ||
| expect(accountContext.getContext()).toEqual({ id: '123' }) | ||
| }) | ||
|
|
||
| it('should set account property', () => { | ||
| accountContext.setContextProperty('foo', 'bar') | ||
| expect(accountContext.getContext()).toEqual({ foo: 'bar' }) | ||
| }) | ||
|
|
||
| it('should remove account property', () => { | ||
| accountContext.setContext({ id: '123', foo: 'bar' }) | ||
| accountContext.removeContextProperty('foo') | ||
| expect(accountContext.getContext()).toEqual({ id: '123' }) | ||
| }) | ||
|
|
||
| it('should clear account', () => { | ||
| accountContext.setContext({ id: '123' }) | ||
| accountContext.clearContext() | ||
| expect(accountContext.getContext()).toEqual({}) |
There was a problem hiding this comment.
These tests are redundant with contextManager.spec.ts
| it('should get context', () => { | ||
| globalContext.setContext({ id: '123' }) | ||
| expect(globalContext.getContext()).toEqual({ id: '123' }) | ||
| }) | ||
|
|
||
| it('should set context property', () => { | ||
| globalContext.setContextProperty('foo', 'bar') | ||
| expect(globalContext.getContext()).toEqual({ foo: 'bar' }) | ||
| }) | ||
|
|
||
| it('should remove context property', () => { | ||
| globalContext.setContext({ id: '123', foo: 'bar' }) | ||
| globalContext.removeContextProperty('foo') | ||
| expect(globalContext.getContext()).toEqual({ id: '123' }) | ||
| }) | ||
|
|
||
| it('should clear context', () => { | ||
| globalContext.setContext({ id: '123' }) | ||
| globalContext.clearContext() | ||
| expect(globalContext.getContext()).toEqual({}) |
There was a problem hiding this comment.
These tests are redundant with contextManager.spec.ts
| userContext = startUserContext(customerDataTrackerManager, mockRumConfiguration()) | ||
| }) | ||
|
|
||
| it('should get user', () => { | ||
| userContext.setContext({ id: '123' }) | ||
| expect(userContext.getContext()).toEqual({ id: '123' }) | ||
| }) | ||
|
|
||
| it('should set user property', () => { | ||
| userContext.setContextProperty('foo', 'bar') | ||
| expect(userContext.getContext()).toEqual({ foo: 'bar' }) | ||
| }) | ||
|
|
||
| it('should remove user property', () => { | ||
| userContext.setContext({ id: '123', foo: 'bar' }) | ||
| userContext.removeContextProperty('foo') | ||
| expect(userContext.getContext()).toEqual({ id: '123' }) | ||
| }) | ||
|
|
||
| it('should clear user', () => { | ||
| userContext.setContext({ id: '123' }) | ||
| userContext.clearContext() | ||
| expect(userContext.getContext()).toEqual({}) |
There was a problem hiding this comment.
These tests are redundant with contextManager.spec.ts
|
|
||
| accountContext.setContextProperty('foo', 'bar') | ||
| expect(accountContext.getContext()).toEqual({ id: 'foo', qux: 'qix', foo: 'bar' }) | ||
| expect(localStorage.getItem('_dd_c_rum_4')).toBe('{"id":"foo","qux":"qix","foo":"bar"}') | ||
|
|
||
| accountContext.removeContextProperty('foo') | ||
| expect(accountContext.getContext()).toEqual({ id: 'foo', qux: 'qix' }) | ||
| expect(localStorage.getItem('_dd_c_rum_4')).toBe('{"id":"foo","qux":"qix"}') | ||
|
|
||
| accountContext.clearContext() | ||
| expect(accountContext.getContext()).toEqual({}) | ||
| expect(localStorage.getItem('_dd_c_rum_4')).toBe('{}') |
There was a problem hiding this comment.
These tests are redundant with contextManager.spec.ts
|
|
||
| globalContext.setContextProperty('foo', 'bar') | ||
| expect(globalContext.getContext()).toEqual({ id: 'foo', qux: 'qix', foo: 'bar' }) | ||
| expect(localStorage.getItem('_dd_c_rum_2')).toBe('{"id":"foo","qux":"qix","foo":"bar"}') | ||
|
|
||
| globalContext.removeContextProperty('foo') | ||
| expect(globalContext.getContext()).toEqual({ id: 'foo', qux: 'qix' }) | ||
| expect(localStorage.getItem('_dd_c_rum_2')).toBe('{"id":"foo","qux":"qix"}') | ||
|
|
||
| globalContext.clearContext() | ||
| expect(globalContext.getContext()).toEqual({}) | ||
| expect(localStorage.getItem('_dd_c_rum_2')).toBe('{}') |
There was a problem hiding this comment.
These tests are redundant with contextManager.spec.ts
|
|
||
| userContext.setContextProperty('foo', 'bar') | ||
| expect(userContext.getContext()).toEqual({ id: 'foo', qux: 'qix', foo: 'bar' }) | ||
| expect(localStorage.getItem('_dd_c_rum_1')).toBe('{"id":"foo","qux":"qix","foo":"bar"}') | ||
|
|
||
| userContext.removeContextProperty('foo') | ||
| expect(userContext.getContext()).toEqual({ id: 'foo', qux: 'qix' }) | ||
| expect(localStorage.getItem('_dd_c_rum_1')).toBe('{"id":"foo","qux":"qix"}') | ||
|
|
||
| userContext.clearContext() | ||
| expect(userContext.getContext()).toEqual({}) | ||
| expect(localStorage.getItem('_dd_c_rum_1')).toBe('{}') |
There was a problem hiding this comment.
These tests are redundant with contextManager.spec.ts
| ;(serverRumEvent.session as Mutable<RumEvent['session']>).has_replay = commonContext.hasReplay | ||
| ;(serverRumEvent.session as Mutable<RumEvent['session']>).has_replay = recorderApi.isRecording() | ||
| ? true | ||
| : undefined |
There was a problem hiding this comment.
I like that this makes the expected behavior clear; this has caused some confusion recently!
| }) | ||
| }) | ||
|
|
||
| it('should not set the account when account.id is undefined', () => { |
There was a problem hiding this comment.
Nit: Worth testing other falsy cases, just to clarify the expected behavior? e.g. id property present but set to undefined, or id set to ''.
There was a problem hiding this comment.
That's a good one and thanks to it I spotted an issue: #3475
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
amortemousque
force-pushed
the
aymeric/add-hook-for-contexts-2
branch
from
4ed22b1 to
265bebf
Compare
amortemousque
force-pushed
the
aymeric/add-hook-for-contexts-2
branch
from
265bebf to
5cea6d2
Compare
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 5cea6d2555 will soon be integrated into staging-15.
We couldn't automatically merge the commit 5cea6d2555 into staging-15. What to do next?
If you think the errors come from a logical conflict with the target branch, you can create a fix by commenting this pull request with DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
Motivation
Use assembly hooks for global, user and account context
Changes
Testing
I have gone over the contributing documentation.