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.

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

fix(browser): Set : as a part of gecko protocol regex group. #4153

Merged
merged 5 commits into from Aug 2, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Nov 12, 2021

Fixes: #4138

The original issue about file keyword also happens for everything in that group, such as http, blob and so on. And the problem originates from : .* between that group and :. It seems that : follows those keywords without anything in between, as far as I have seen from the tests.

Removing that from the regex solved the issue without breaking any tests other than safari-extension and safari-web-extension, which are special-cased here, adding those two to the matching group also solved the issue of an extra : coming from there.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2021

size-limit report

Path Base Size (44e0841) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 20.24 KB 20.25 KB +0.04% 🔺
@sentry/browser - CDN Bundle (minified) 64.29 KB 64.32 KB +0.06% 🔺
@sentry/browser - Webpack 22.22 KB 22.22 KB +0.02% 🔺
@sentry/browser - Webpack - gzip = false 75.7 KB 75.74 KB +0.05% 🔺
@sentry/react - Webpack 22.25 KB 22.26 KB +0.02% 🔺
@sentry/nextjs Client - Webpack 46.35 KB 46.36 KB +0.02% 🔺
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.34 KB 28.35 KB +0.04% 🔺

@AbhiPrasad
Copy link
Member

We can cut a beta and test this - no need to enforce that it needs to be part of the next immediate release.

Also, we only tested latest firefox back when we were using BrowserStack regularly, I think as long as we can validate with playwright that this doesn't break anything, we should be good!

bs_firefox: {
base: "BrowserStack",
browser: "Firefox",
browser_version: "latest",
os: "Windows",
os_version: "10",
device: null,
real_mobile: null,

@AbhiPrasad
Copy link
Member

I'm going to set this as blocked, let's get the e2e tests in first, and then try to merge this in.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2021

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@AbhiPrasad
Copy link
Member

@onurtemizkan can we rebase and add an integration test for this now?

@onurtemizkan onurtemizkan force-pushed the onur/gecko-stacktraces branch 3 times, most recently from fd20fd0 to 2490499 Compare December 19, 2021 17:19
@onurtemizkan onurtemizkan force-pushed the onur/gecko-stacktraces branch 2 times, most recently from 3c1f04d to 537da47 Compare December 21, 2021 15:42
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

Looks pretty good! My feedback is mostly just making sure things are clearer for the reader.

@AbhiPrasad
Copy link
Member

@onurtemizkan is it possible to re-create this PR when you get some time? The stacktrace parsing logic has moved, so probably not worth it to rebase the PR.

@onurtemizkan onurtemizkan changed the base branch from master to 7.x April 29, 2022 09:02
@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.36 KB (+0.04% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 59.95 KB (+0.06% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.93 KB (+0.04% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 52.83 KB (+0.07% 🔺)
@sentry/browser - Webpack (gzipped + minified) 19.7 KB (+0.03% 🔺)
@sentry/browser - Webpack (minified) 64.17 KB (+0.06% 🔺)
@sentry/react - Webpack (gzipped + minified) 19.72 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.11 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.77 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.02 KB (+0.03% 🔺)

@AbhiPrasad
Copy link
Member

@onurtemizkan - just so we can finally close the loop on this. Mind rebasing this for a final time? Let's get it merged in!

@onurtemizkan onurtemizkan force-pushed the onur/gecko-stacktraces branch 2 times, most recently from 49cfc13 to b17ad68 Compare July 27, 2022 23:55
@onurtemizkan onurtemizkan changed the base branch from 7.x to master July 27, 2022 23:56
@AbhiPrasad
Copy link
Member

@onurtemizkan mind rebasing this PR? We had to fix some NextJS test failures with #5484

@AbhiPrasad
Copy link
Member

^ Bump to rebase this, let's get it merged in!

@lobsterkatie
Copy link
Member

^ Bump to rebase this, let's get it merged in!

On it!

@lobsterkatie lobsterkatie merged commit cb925e3 into master Aug 2, 2022
@lobsterkatie lobsterkatie deleted the onur/gecko-stacktraces branch August 2, 2022 15:41
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.

Stack trace is incorrectly parsed, concatenates identifier with file path [Firefox]
3 participants