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.

Sign up for GitHub

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

Jump to bottom

fix(normalize): Treat Infinity as NaN both are non-serializable numbers #13406

Merged
merged 9 commits into from
Sep 12, 2024
Merged

fix(normalize): Treat Infinity as NaN both are non-serializable numbers #13406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b0434e2
fix(normalize): Treat Infinity as NaN both are non-serializable numbers
krystofwoldrich Aug 16, 2024
aa096bd
Merge remote-tracking branch 'origin/develop' into kw/normalize-infinity
krystofwoldrich Aug 26, 2024
f452357
Merge remote-tracking branch 'origin/develop' into kw/normalize-infinity
krystofwoldrich Aug 27, 2024
ac9939e
fix infinity normalization
krystofwoldrich Aug 27, 2024
39c1249
use one if for both positive and negative
krystofwoldrich Aug 27, 2024
61b1554
remove explicit NaN check as NaN is not finite
krystofwoldrich Aug 28, 2024
a32c4e8
Merge remote-tracking branch 'origin/develop' into kw/normalize-infinity
krystofwoldrich Sep 10, 2024
f664a5f
fix ee2e tests
krystofwoldrich Sep 10, 2024
d265d92
fix lint
krystofwoldrich Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion dev-packages/browser-integration-tests/suites/public-api/setExtras/consecutive_calls/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,10 @@ sentryTest('should set extras from multiple consecutive calls', async ({ getLoca
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

expect(eventData.message).toBe('consecutive_calls');
expect(eventData.extra).toMatchObject({ extra: [], Infinity: 2, null: null, obj: { foo: ['bar', 'baz', 1] } });
Copy link
Member Author

Choose a reason for hiding this comment

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

The value under null key is Infinity

sentry-javascript/dev-packages/browser-integration-tests/suites/public-api/setExtras/consecutive_calls/subject.js

Line 8 in f664a5f

Sentry.setExtras({ [null]: Infinity });

expect(eventData.extra).toMatchObject({
extra: [],
Infinity: 2,
null: '[Infinity]',
obj: { foo: ['bar', 'baz', 1] },
});
});
7 changes: 4 additions & 3 deletions packages/utils/src/normalize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ function visit(
// Get the simple cases out of the way first
if (
value == null || // this matches null and undefined -> eqeq not eqeqeq
(['number', 'boolean', 'string'].includes(typeof value) && !Number.isNaN(value))
['boolean', 'string'].includes(typeof value) ||
(typeof value === 'number' && Number.isFinite(value))
) {
return value as Primitive;
}
Expand Down Expand Up @@ -220,8 +221,8 @@ function stringifyValue(
return '[SyntheticEvent]';
}

if (typeof value === 'number' && value !== value) {
return '[NaN]';
if (typeof value === 'number' && !Number.isFinite(value)) {
Copy link
Member

Choose a reason for hiding this comment

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

actually, we can improve this even further :D we can combine this with the check above for nan, as (NaN).toString() also returns NaN. And isFinite(NaN) === false. So we can remove this completely:

if (typeof value === 'number' && value !== value) {
      return '[NaN]';
    }

and only keep the new check you added!

return `[${value}]`;
}

if (typeof value === 'function') {
Expand Down
2 changes: 2 additions & 0 deletions packages/utils/test/normalize.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,8 @@ describe('normalize()', () => {
describe('changes unserializeable/global values/classes to their respective string representations', () => {
test('primitive values', () => {
expect(normalize(NaN)).toEqual('[NaN]');
expect(normalize(Infinity)).toEqual('[Infinity]');
expect(normalize(-Infinity)).toEqual('[-Infinity]');
expect(normalize(Symbol('dogs'))).toEqual('[Symbol(dogs)]');
expect(normalize(BigInt(1121201212312012))).toEqual('[BigInt: 1121201212312012]');
});
Expand Down
Loading