Skip to content

fix(node): Record local variables with falsy values, null and undefined #10821

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
Merged
Lms24 merged 5 commits into from
Feb 27, 2024
Merged

fix(node): Record local variables with falsy values, null and undefined #10821

Lms24 merged 5 commits into from
Feb 27, 2024

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 27, 2024 •
edited
Loading

We previously didn't record local local variables with falsy values because we checked for truthiness instead of definedness when unrolling and serializing the variable values. This PR changes how we check for and extract variable properties to record

  • falsy values (0, '', false)
  • null
  • undefined

Fixes implementations in node and node-experimental

fixes #10815

@Lms24 Lms24 changed the title fix(node): Record local variables if they're falsy fix(node): Record local variables with falsy values Feb 27, 2024
@Lms24 Lms24 changed the title fix(node): Record local variables with falsy values fix(node): Record local variables with falsey values Feb 27, 2024
@Lms24 Lms24 changed the title fix(node): Record local variables with falsey values fix(node): Record local variables with falsy values Feb 27, 2024
Lms24
Lms24 commented Feb 27, 2024
View reviewed changes
dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
obj: { name: 'some name', num: 5 },
ty: '<Some>',
bool: false,
num: 0,
Copy link
Member Author

@Lms24 Lms24 Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In case of a number, we would previously report '<0>' instead of 0. I think changing this should be okay but wanted to double check if there are implications to this change. Anyone have an idea?

@Lms24 Lms24 requested review from AbhiPrasad and timfish February 27, 2024 10:13
@github-actions
Copy link
Contributor

github-actions bot commented Feb 27, 2024 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.3 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.56 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.47 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.06 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.74 KB (+0.07% 🔺)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 32.74 KB (+0.07% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.91 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 30.91 KB (0%)
@sentry/browser - Webpack (gzipped) 22.19 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.71 KB (-0.06% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.47 KB (-0.04% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.25 KB (-0.11% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.74 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 211.07 KB (-0.1% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 99.79 KB (-0.21% 🔽)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 73.96 KB (+0.25% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.3 KB (-0.15% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.84 KB (+0.04% 🔺)
@sentry/react - Webpack (gzipped) 22.22 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.3 KB (+0.06% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 49.68 KB (+0.05% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 17.14 KB (0%)

Lms24
Lms24 commented Feb 27, 2024
View reviewed changes
Copy link
Member Author

@Lms24 Lms24 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lol one thing I'm realizing here: This change still filters out variables that are simply assigned undefined or null. Should we completely remove the definedness checks?

@HazAT
Copy link
Member

HazAT commented Feb 27, 2024

toString it 😅?

@Lms24 Lms24 self-assigned this Feb 27, 2024
@Lms24 Lms24 changed the title fix(node): Record local variables with falsy values fix(node): Record local variables with falsy values, null and undefined Feb 27, 2024
@Lms24 Lms24 mentioned this pull request Feb 27, 2024
Closed
3 tasks
@Lms24 Lms24 merged commit 519aacc into develop Feb 27, 2024
@Lms24 Lms24 deleted the lms/fix-node-localvariables-falsy-values branch February 27, 2024 15:29
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 18, 2024

Should we backport this to v7?

This was referenced Mar 18, 2024
Closed
Merged
AbhiPrasad pushed a commit that referenced this pull request Mar 19, 2024
@timfish
fix(node): Record local variables with falsy values (v7) (#11190)
c7306da 
Backport to v7 of #10821
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timfish timfish timfish approved these changes

@mydea mydea mydea approved these changes

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

Assignees

@Lms24 Lms24

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Local variables not recorded when they are falsey

6 participants

@Lms24 @HazAT @AbhiPrasad @timfish @mydea