Skip to content

Commit 679a02c

Browse files
authored
ref(core): Optimize Scope.setTag bundle size and adjust test (#18182)
Two changes: 1. Reduce bundle size slightly by optimizing `setTag` (+ adding some more tests around setTag(s)) 2. Adjust the integration test message since we no longer classify the SUT behaviour as a bug
1 parent 0f4c190 commit 679a02c

File tree

3 files changed

+49
-15
lines changed

3 files changed

+49
-15
lines changed

dev-packages/browser-integration-tests/suites/public-api/setTag/with_non_primitives/test.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,21 @@ import type { Event } from '@sentry/core';
33
import { sentryTest } from '../../../../utils/fixtures';
44
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
55

6-
sentryTest('[bug] accepts non-primitive tags', async ({ getLocalTestUrl, page }) => {
7-
// this is a bug that went unnoticed due to type definitions and a bad assertion
8-
// TODO: We should not accept non-primitive tags. Fix this as a follow-up.
6+
sentryTest('accepts and sends non-primitive tags', async ({ getLocalTestUrl, page }) => {
7+
// Technically, accepting and sending non-primitive tags is a specification violation.
8+
// This slipped through because a previous version of this test should have ensured that
9+
// we don't accept non-primitive tags. However, the test was flawed.
10+
// Turns out, Relay and our product handle invalid tag values gracefully.
11+
// Our type definitions for setTag(s) also only allow primitive values.
12+
// Therefore (to save some bundle size), we'll continue accepting and sending non-primitive
13+
// tag values for now (but not adjust types).
14+
// This test documents this decision, so that we know why we're accepting non-primitive tags.
915
const url = await getLocalTestUrl({ testDir: __dirname });
1016

1117
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
1218

1319
expect(eventData.message).toBe('non_primitives');
1420

15-
// TODO: This should be an empty object but instead, it is:
1621
expect(eventData.tags).toEqual({
1722
tag_1: {},
1823
tag_2: [],

packages/core/src/scope.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -291,9 +291,7 @@ export class Scope {
291291
* Set a single tag that will be sent as tags data with the event.
292292
*/
293293
public setTag(key: string, value: Primitive): this {
294-
this._tags = { ...this._tags, [key]: value };
295-
this._notifyScopeListeners();
296-
return this;
294+
return this.setTags({ [key]: value });
297295
}
298296

299297
/**

packages/core/test/lib/scope.test.ts

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,16 +140,47 @@ describe('Scope', () => {
140140
expect(scope['_extra']).toEqual({ a: undefined });
141141
});
142142

143-
test('setTag', () => {
144-
const scope = new Scope();
145-
scope.setTag('a', 'b');
146-
expect(scope['_tags']).toEqual({ a: 'b' });
143+
describe('setTag', () => {
144+
it('sets a tag', () => {
145+
const scope = new Scope();
146+
scope.setTag('a', 'b');
147+
expect(scope['_tags']).toEqual({ a: 'b' });
148+
});
149+
150+
it('sets a tag with undefined', () => {
151+
const scope = new Scope();
152+
scope.setTag('a', 'b');
153+
scope.setTag('a', undefined);
154+
expect(scope['_tags']).toEqual({ a: undefined });
155+
});
156+
157+
it('notifies scope listeners once per call', () => {
158+
const scope = new Scope();
159+
const listener = vi.fn();
160+
161+
scope.addScopeListener(listener);
162+
scope.setTag('a', 'b');
163+
scope.setTag('a', 'c');
164+
165+
expect(listener).toHaveBeenCalledTimes(2);
166+
});
147167
});
148168

149-
test('setTags', () => {
150-
const scope = new Scope();
151-
scope.setTags({ a: 'b' });
152-
expect(scope['_tags']).toEqual({ a: 'b' });
169+
describe('setTags', () => {
170+
it('sets tags', () => {
171+
const scope = new Scope();
172+
scope.setTags({ a: 'b', c: 1 });
173+
expect(scope['_tags']).toEqual({ a: 'b', c: 1 });
174+
});
175+
176+
it('notifies scope listeners once per call', () => {
177+
const scope = new Scope();
178+
const listener = vi.fn();
179+
scope.addScopeListener(listener);
180+
scope.setTags({ a: 'b', c: 'd' });
181+
scope.setTags({ a: 'e', f: 'g' });
182+
expect(listener).toHaveBeenCalledTimes(2);
183+
});
153184
});
154185

155186
test('setUser', () => {

0 commit comments

Comments
 (0)