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

feat(with-sentry): update with-sentry examples to align with shared environments #70

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

vernak2539
Copy link
Contributor

@vernak2539 vernak2539 commented Sep 23, 2024

I went through a lot of variations when upgrading/using Sentry in extensions.

While Sentry does provide guidance when it comes to shared environments, recommending v8, this guidance really only applies to code executed in the content scripts. This is because content scripts are the only thing executed in the same window as the website currently in the browser.

But, what I found out the hard way is that even using Sentry's default way of initialising in the popup script can cause issues, specifically, rejections from the stores.

Violation: Including remotely-hosted code in a Manifest V3 item.

With this code snippet being the one in question.

Code snippet: popup.cc3817b7.js: " let u = function(e) { let t = (0, o.getClient)(), r = t && t.getOptions(), n = r && r.cdnBaseUrl || "https://browser.sentry-cdn.com/"; return new URL(`/${(0,o.SDK_VERSION)}/${e}.min.js`, n).toString() }(r), c = (0, s.WINDOW).document.createElement("script"); c.src = u, c.crossOrigin = "anonymous", c.referrerPolicy = "origin", t && c.setAttribute("nonce", t); let p = new Promise((e, t) => { c.addEventListener("load", () => e()), c.addEventListener("error", t) }), d = s.WINDOW.document.currentScript,"

After a deep-dive, this code is included when using the Sentry.init function. This is because of how the import works (e.g. import * as Sentry from '@sentry/browser [or from the react version]), as it actually imports all things, which includes the lazy loading integration [this integration isn't needed when you're bundling code, but hey ho).

I've modified the example to show how to set up Sentry for usage in a content script, which shouldn't have this problem, but I've left the popup initialisation as standard with a comment. Open for discussion!

@vernak2539
Copy link
Contributor Author

@louisgv one more please whenever you have a moment?

Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Minor nit but main thing is the dependency freeze

with-sentry/package.json Outdated Show resolved Hide resolved
with-sentry/sentry.ts Outdated Show resolved Hide resolved
@vernak2539
Copy link
Contributor Author

@louisgv I've updated this PR. If you wouldn't mind giving it a final review and merging if you see fit

@vernak2539
Copy link
Contributor Author

@louisgv sorry to keep on poking, but this is ready

Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Lgtm!

@louisgv louisgv merged commit 9777df4 into PlasmoHQ:main Oct 30, 2024
@vernak2539 vernak2539 deleted the improve-sentry branch October 30, 2024 08:35
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.

2 participants