Skip to content

ref(utils): use test instead of indexof in stacktrace#7417

Merged
AbhiPrasad merged 4 commits intodevelopfrom
jb/ref/stacktrace-matching
Mar 14, 2023
Merged

ref(utils): use test instead of indexof in stacktrace#7417
AbhiPrasad merged 4 commits intodevelopfrom
jb/ref/stacktrace-matching

Conversation

@JonasBa
Copy link
Copy Markdown
Member

@JonasBa JonasBa commented Mar 10, 2023

We dont require the actual index + we can match with a single fn call

@JonasBa JonasBa requested review from AbhiPrasad and timfish March 10, 2023 20:16
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 10, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.23 KB (+0.05% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 62.93 KB (+0.06% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.86 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55.84 KB (+0.07% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.61 KB (+0.07% 🔺)
@sentry/browser - Webpack (minified) 67.36 KB (+0.05% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.64 KB (+0.08% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.53 KB (+0.16% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.49 KB (+0.16% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.75 KB (+0.15% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.17 KB (0%)
@sentry/replay - Webpack (gzipped + minified) 37.19 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.16 KB (+0.06% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 54.34 KB (+0.01% 🔺)

.reverse();
return localStack.map(frame => ({
...frame,
filename: frame.filename || localStack[0].filename,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am a bit puzzled why this has a fallback to localStack[0].filename?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure - this seems strange to me, and feels wrong. Shouldn't we just drop the filename if it's not set?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, that is what I would assume. It seems like we were defaulting to the file at the root of the stack?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess for now let's just keep the behaviour, but I'll make a TODO to come back and investigate this.

@JonasBa
Copy link
Copy Markdown
Member Author

JonasBa commented Mar 10, 2023

OK, I'm having some trouble with fixing this test... Even if I revert my implementation back to the original it still fails locally? I'm not entirely sure why this would be the case or if I'm maybe using this wrongly? I noticed that since the test depends on a different package I wanted to rebuild the package however I still cant see any logs 😢

@JonasBa JonasBa force-pushed the jb/ref/stacktrace-matching branch from 678dcce to e4e9db2 Compare March 10, 2023 23:07
@AbhiPrasad
Copy link
Copy Markdown
Contributor

Let's merge this in for now and come back to take a look at the filename.

@AbhiPrasad AbhiPrasad merged commit a34dab6 into develop Mar 14, 2023
@AbhiPrasad AbhiPrasad deleted the jb/ref/stacktrace-matching branch March 14, 2023 16:22
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.

3 participants