-
-
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
feat(js): Add SDK Options page #12502
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
||
type Props = { | ||
children?: React.ReactNode; | ||
noGuides?: boolean; | ||
notSupported?: string[]; | ||
notSupported?: `${PlatformCategory}`[]; |
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.
Actually I wonder if we can remove that enum and just use string literals there overall, could not find a place where this was really used that way, but not sure... cc @chargome
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 getting rid of enums per se I think is a good idea, also could't spot anything where this would be actually used
7dc93a6
to
9cbb304
Compare
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 really like this rewrite! IMHO, this is much cleaner than the previous "Basic Options" page, both in description as well as UI. Love the table with the type and default value information.
I read through the options (haven't done this in a while) and had some minor suggestions. Once this is out of draft state, I'm happy to ✅ !
|
||
<SdkOption name="debug" type='boolean' defaultValue='false'> | ||
|
||
Turns debug mode on or off. If debug is enabled SDK will attempt to print out useful debugging information if something goes wrong with sending the event. The default is always `false`. It's generally not recommended to turn it on in production, though turning `debug` mode on will not cause any safety concerns. |
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.
safety sounds weird here 🤔 either we say "security" or write something like it won't expose any critical information? Or just omit it?
Turns debug mode on or off. If debug is enabled SDK will attempt to print out useful debugging information if something goes wrong with sending the event. The default is always `false`. It's generally not recommended to turn it on in production, though turning `debug` mode on will not cause any safety concerns. | |
Turns debug mode on or off. If debug is enabled SDK will attempt to print out useful debugging information if something goes wrong with sending the event. The default is always `false`. It's generally not recommended to turn it on in production, though turning `debug` mode on will not cause any security concerns. |
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.
maybe we should just remove the last sentence here, not sure how relevant this is actually..?
|
||
</SdkOption> | ||
|
||
<SdkOption name="tracePropagationTargets" type='Array<string | RegExp>'> |
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'd also add a default value entry into the table. Which needs to be different on browser and server.
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 this is a bit tricky because 1. it is hard to implement this nicely (we can, if we want to) and especially 2. the default value here is not an actual value but computed (e.g. based on the current host), which is why I opted to rather explain the default in this case?
`proxy`: A proxy used for outbound requests. Can be http or https. - | ||
`caCerts`: A path or list of paths to a CA certificate, or a buffer of CA | ||
certificates. - `httpModule`: A custom HTTP module to use for requests. | ||
Defaults to the the native `http` and `https` modules. - `keepAlive`: | ||
Determines whether to keep the socket alive between requests. Defaults to | ||
`false`. | ||
</PlatformCategorySection> | ||
|
||
<PlatformCategorySection supported={["browser", "desktop"]}> | ||
- `headers`: An object containing headers to be sent with every request. Used | ||
by the SDK's fetch and XHR transports. - `fetchOptions`: An object containing |
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.
are we intentionally not putting the individual transport options into their own lines?
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.
no! auto-format messed this up, good catch!
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.
Nice change!
src/components/sdkOption.tsx
Outdated
</a> | ||
</h3> | ||
|
||
<table className="w-auto"> |
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.
WDYT about wrapping this table with a border radius? (same value as the alert)
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 updated it, it's a bit tricky because apparently border-collapse and border-radius do not like each other 😬 I ended up making the border 2px, which seems ok to me, but... 🤷
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.
Hmm tricky, the prose
class also makes this harder. Looks better with 1px imo and we need to adapt it to dark mode – I can do that tomorrow if you want!
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.
some more hacking around, figured out a way to make 1px borders work!
|
||
type Props = { | ||
children?: React.ReactNode; | ||
noGuides?: boolean; | ||
notSupported?: string[]; | ||
notSupported?: `${PlatformCategory}`[]; |
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 getting rid of enums per se I think is a good idea, also could't spot anything where this would be actually used
That looks much clearer! |
I've thought about this too some, IMHO it makes sense to have them manually sorted - right now it is just the order that you put them into the mdx file! Once we have general agreement on the direction etc. I would take some time to reorder these better, for now I mostly just cobbled it together in the order it already was in basic options. |
Co-authored-by: Charly Gomez <[email protected]>
Bundle ReportChanges will increase total bundle size by 9.28kB (0.05%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-client-array-pushAssets Changed:
Files in
Files in
view changes for bundle: sentry-docs-server-cjsAssets Changed:
Files in
Files in
App Routes Affected:
|
Co-authored-by: Lukas Stracke <[email protected]>
After talking some about this with @lforst and @Lms24, and triggered by talks with @inventarSarah, I decided to make a POC of a new SDK Options page for the JS SDKs.
This was borne out of the following thoughts:
Configuration > Basic Options
, which is a bit weird. First, it is pretty generic (originally applying to all SDKs, so containing a lot of generic language). Second, it is not fully up to date. And lastly, it is not really complete - some (many) options are described but not all.To improve on this, this PR introduces (for JS only, for now) a new top-level "SDK Options" nav item. This is a single page that contains all top-level SDK options that can be passed to
Sentry.init()
. There are a few goals here:For this, I wrote two new components, a
<SdkOption>
component, and a<SdkOptionOverview>
which just auto-generates an index for this. You can put headings and text between those for sectioning and explanation.I also removed the old Basic options page as well as the transports page and folded that into the new page, adding redirects.
Some screenshots showing the different things:
Auto-generated Overview:
Option with env-var configuration:
Note that the
env var
entry will not be shown in environments where this is not auto-picked up.Option with default value:
If in a meta-framework, options that are only available in one part or the other will be called out:
We can add more things to the SDK options if needed. The authoring experience with this should be pretty straightforward and make it easy to keep this up-to-date, hopefully.
Follow up
If this generally finds agreement, we can merge this. We should then make some passes over the actual options and make sure we add ones that have been forgotten, and/or improve docs on them where applicable. In some places, we have docs for options in more specific places, which should be folded into this page where possible.
When this is done, we would like to also add a top-level
SDK APIs
page, which does the same thing but for APIs, e.g.Sentry.captureException()
,Sentry.setTag()
etc. We can discuss this more separately.The idea is that with these two pages, we'll be able to consolidate a good amount of the pages we currently have in various levels of nesting, and only keep additional pages for places where more explanation/examples are needed.