-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
docs(js): Update content in SvelteKit's manual quick start guide (source maps, csp) #13843
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The CSP section looks great already. I had a concern about the source maps configuration settings (see comments). TLDR: We should never set the auth token in code but only as env variables.
Realizing that the comment order in my review is a bit misleading for source maps. I recommend reading #13843 (comment) first :)
platform-includes/sourcemaps/upload/primer/javascript.sveltekit.mdx
Outdated
Show resolved
Hide resolved
platform-includes/sourcemaps/upload/primer/javascript.sveltekit.mdx
Outdated
Show resolved
Hide resolved
platform-includes/sourcemaps/upload/primer/javascript.sveltekit.mdx
Outdated
Show resolved
Hide resolved
However, you still need to specify your Sentry auth token as well as your org and project slugs. | ||
There are two ways to set them: | ||
|
||
**Option 1** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: The options here really only differ in how much users set as env variables vs. directly in code. Users can set project and org slugs in code but they should never set the auth token in code as it must remain a secret and shouldn't be committed.
So I'd recommend we:
- always show the
.env
file for setting theSENTRY_AUTH_TOKEN
- also show a complete
.env
file for option 1 (see my comment) - show both
.env
andvite.config.ts
for option 2 (basically how we used to show it in manual setup).
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this up -- I think I understood what you meant -> let me know if you have feedback on the changes.
I'll review the other SDKs to see whether we need to update this there as well next week. It would be ideal to be consistent here.
platform-includes/sourcemaps/upload/primer/javascript.sveltekit.mdx
Outdated
Show resolved
Hide resolved
platform-includes/sourcemaps/upload/primer/javascript.sveltekit.mdx
Outdated
Show resolved
Hide resolved
…eltekit/clean-up-manual-qs
Bundle ReportChanges will increase total bundle size by 465 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-client-array-pushAssets Changed:
view changes for bundle: sentry-docs-server-cjsAssets Changed:
|
sourceMapsUploadOptions: { | ||
org: "___ORG_SLUG___", | ||
project: "___PROJECT_SLUG___", | ||
authToken: "process.env.SENTRY_AUTH_TOKEN", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be a string:
authToken: "process.env.SENTRY_AUTH_TOKEN", | |
authToken: process.env.SENTRY_AUTH_TOKEN, |
|
||
You can also set your org and project slugs by passing a `sourceMapsUploadOptions` object to `sentrySvelteKit`, as seen in the example below. For a full list of available options, see the [Sentry Vite Plugin documentation](https://www.npmjs.com/package/@sentry/vite-plugin#options). | ||
|
||
<OrgAuthTokenNote /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should still show the auth token in the .env file here, given that the token needs to be set somewhere. Otherwise, we should at least remove the <OrgAuthTokenNote>
here.
<OrgAuthTokenNote /> | |
<OrgAuthTokenNote /> | |
\```bash {filename:.env} | |
# DO NOT commit this file to your repo. The auth token is a secret. | |
SENTRY_AUTH_TOKEN=___ORG_AUTH_TOKEN___ | |
\``` | |
(please ignore the backslashes, GH makes suggesting code blocks impossible :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I added the .env file here as suggested - thank you!
In Option 1, I also added OrgAuthTokenNote above the .env file
ignore: ["**/build/client/**/*"], | ||
filesToDeleteAfterUpload: ["./build/**/*.map"], | ||
}, | ||
authToken: "___ORG_AUTH_TOKEN___", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized we have the same problem here. We also shouldn't directly put the auth token into code here. I actually liked the previous version where we basically showed option 2. Happy to leave it up to you if it makes more sense to display both files in tabs in the same code block or one underneath the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Updated this section and added the .env with the auth token.
In general, it is best to provide only one option in a quick start guide, allowing the user to focus on setting things up quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Please address the step number comment before merging
…eltekit/clean-up-manual-qs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including my suggestions! LGTM!
…rce maps, csp) (#13843) <!-- Use this checklist to make sure your PR is ready for merge. You may delete any sections you don't need. --> ## DESCRIBE YOUR PR In this PR I have: - reduced the source maps content in SvelteKit's manual quick start guide and moved it to the Source Maps page - moved content about CSP to the Troubleshooting page and added an Expandable to the manual quick start guide I will move the build options and API content in a new PR (or two) to keep things focused. Belongs to: #13634 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [x] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs) ## EXTRA RESOURCES - [Sentry Docs contributor guide](https://docs.sentry.io/contributing/)
DESCRIBE YOUR PR
In this PR I have:
I will move the build options and API content in a new PR (or two) to keep things focused.
Belongs to: #13634
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes:
EXTRA RESOURCES