-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
docs(astro): Update getting started page #11953
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Bundle ReportChanges will increase total bundle size by 15 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
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.
Generally works for me! I'll let @Lms24 also give it a pass though.
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 taking this on, Charly! General thoughts:
- I'm okay with removing manual setup and merging it with Getting Started as long as we don't have a wizard. Which in all likelihood we won't have for Astro.
- I'd like us to continue using Astro's CLI to add the Astro integration because it's just what users are used to do. Unless there's a good reason not to. Definitely open to hearing your thoughts on this though!
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/h: What is our rationale behind moving away from npx astro add @sentry/astro
? Despite us instructing people to declare separate client|server config files, the astro CLI would still automatically register the intgration in astro.config.mjs
for users.
Users are probably familiar with adding Astro integrations and we're also listed in their official integrations directory: https://astro.build/integrations/?search=sentry
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.
good point, I'll update that
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.
the only thing that bugged me here is that we have another code snippet for adding profiling
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.
l/no action required: I find it a bit unfortunate that the advanced config section is also on the Getting Started page. We might want to consider moving this somewhere else. Previously, "Manual Setup" gave us the option to write all of this down but still have a short and concise "Getting Started" page. I think this might be worth to revisit later, potentially after the rework on the "Getting Started" pages (contrator?) of other SDKs is in a good state 🤔
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.
yeah was not sure where to put this tbh, I guess just adding an advanced config sub page also would not 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.
@Lms24 merging this so we get this out for now, we can do a follow up!
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.
sorry, for not reviewing this in time! I'm fine with leaving it as-is for the moment
}); | ||
``` | ||
|
||
### Server-side Setup |
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.
### Server-side Setup | |
### Server-Side Setup |
platform-includes/getting-started-next-steps/javascript.astro.mdx
Outdated
Show resolved
Hide resolved
platform-includes/getting-started-next-steps/javascript.astro.mdx
Outdated
Show resolved
Hide resolved
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 the updates! Added a couple suggestions, looks good to me! 🫡🦃
Co-authored-by: Alex Krawiec <[email protected]>
Co-authored-by: Alex Krawiec <[email protected]>
Co-authored-by: Alex Krawiec <[email protected]>
documents getsentry/sentry-javascript#14274