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: Safeguard for Sentry default templates + nitpick #9012

Merged
merged 6 commits into from
Dec 19, 2023
Merged

Conversation

HazAT
Copy link
Contributor

@HazAT HazAT commented Aug 6, 2023

Hey,
I added a small safeguard to not break the frontend once a user added Sentry.
process is undefined if a user doesn't add it to redwood.toml, and it results in the default rendering a blank page.
Before
Pasted image 20230806165726

This fix adds a safeguard and some barebone instructions on what a user has to do.
After
Pasted image 20230806171504

  • Also added a fix for @sentry/tracing imports; We recently deprecated the need to import it from there separately
  • Changed the default value of the DSN from https://[email protected]/XXXXXXX to '' (empty string) because the other one might result in an error
  • Add HTTP Tracing integration to continue the trace from the frontend

@jtoar jtoar added the release:fix This PR is a fix label Aug 7, 2023
@jtoar jtoar modified the milestones: next-release, next-release-patch Aug 7, 2023
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Hey @HazAT, yeah we could do with a better error, the "process is undefined" one leaves a lot to be desired. Left a comment on the conditional, let me know what you think. Thanks!

let dsn = ''
let environment = 'development'

if (typeof process === 'undefined' || !process.env.SENTRY_DSN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The way Vite works (on Redwood v6), it replaces whole "property access" chains (I'm not sure what the word is), like process.env.SENTRY_DSN. The process object doesn't really exist. So I think process will always be undefined. Anyway, the upshot is that it's probably better to just check the second one:

Suggested change
if (typeof process === 'undefined' || !process.env.SENTRY_DSN) {
if (!process.env.SENTRY_DSN) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it just with just the second condition but it still throws Uncaught ReferenceError: process is not defined
tbf I only tested in dev and not a prod build.

@jtoar jtoar modified the milestones: next-release-patch, v6.0.6 Aug 11, 2023
@jtoar jtoar modified the milestones: next-release-patch, v6.1.1 Sep 2, 2023
@jtoar jtoar modified the milestones: next-release-patch, v6.2.1 Sep 13, 2023
@jtoar jtoar modified the milestones: next-release-patch, v6.3.2 Oct 5, 2023
@Tobbe Tobbe self-assigned this Dec 17, 2023
@Tobbe
Copy link
Member

Tobbe commented Dec 17, 2023

Thanks again for your PR @HazAT 🙏 Sorry it got stuck 🙁

A couple of the changes were addressed in this recent PR: #9542 But other changes here are still valuable! I'll see what I can do about getting this merged.

@Tobbe Tobbe merged commit b9f8c97 into redwoodjs:main Dec 19, 2023
Tobbe added a commit that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants