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

Add guide for web workers. #3562

Merged
merged 9 commits into from
Jun 8, 2021

Conversation

onurtemizkan
Copy link
Collaborator

Adds simple documentation on Web Workers support.

@onurtemizkan onurtemizkan requested a review from rhcarvalho as a code owner May 11, 2021 07:03
@vercel
Copy link

vercel bot commented May 11, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sentry/sentry-docs/BKSytNwnRkFvLuiEA5dRg17PfBxN
✅ Preview: https://sentry-docs-git-fork-onurtemizkan-add-web-workers-guide.sentry.dev

src/platforms/javascript/guides/webworkers/index.mdx Outdated Show resolved Hide resolved
src/platforms/javascript/guides/webworkers/index.mdx Outdated Show resolved Hide resolved
src/platforms/javascript/guides/webworkers/index.mdx Outdated Show resolved Hide resolved
src/platforms/javascript/guides/webworkers/index.mdx Outdated Show resolved Hide resolved
@PeloWriter
Copy link
Contributor

Minor smoothing - looks good. Thanks, @onurtemizkan

@rhcarvalho
Copy link
Contributor

@PeloWriter do you think this fits in as a guide in the structure? The way I understand it this is just a particular environment where one may use the regular JavaScript browser SDK, we use guides for frameworks and custom SDKs.

@PeloWriter
Copy link
Contributor

Ah, I had understood it was a guide - a framework or custom SDK, as you note. Since it's an environment that's available for all the JS SDK family, I'd suggest we maintain it configuration content.

@onurtemizkan - if you and @rhcarvalho agree, let's make this part of javascript/common/configuration

@onurtemizkan
Copy link
Collaborator Author

Thanks, @PeloWriter, @rhcarvalho. I moved it to javascript/common/configuration. Is there a specific format to follow in that part, or could we keep this in guide format?

@PeloWriter
Copy link
Contributor

The format looks fine to me. For configuration, we generally introduce what can be configured, then provide code samples.

@rhcarvalho
Copy link
Contributor

I'd start putting this content out for the browser SDK only, not any variants, thus not common content. Not until we validate it works and makes sense for all places the common content goes into.

@PeloWriter
Copy link
Contributor

In that case, we can keep this where it is, just adjust the yaml frontmatter to list the SDKs/guides for which Web Workers isn't supported. That way, as support expands, there's no need to move the content - we'll just remove the SDK/guide from the list of notSupported.

@onurtemizkan - here's an example of how that looks in the yaml frontmatter:

notSupported:
 - javascript.cordova

@onurtemizkan
Copy link
Collaborator Author

@PeloWriter, @rhcarvalho I'm currently investigating possible workarounds for currently unsupported functionality. When I get confirmation/validation for a feasible approach, I'll update the PR to cover it.

@PeloWriter
Copy link
Contributor

Thanks for the update, @onurtemizkan. Do you want to turn this to a draft until then?

@onurtemizkan
Copy link
Collaborator Author

Yes, @PeloWriter. Thanks.

@onurtemizkan onurtemizkan marked this pull request as draft May 19, 2021 16:03
@onurtemizkan onurtemizkan marked this pull request as ready for review May 25, 2021 16:09
@onurtemizkan
Copy link
Collaborator Author

Added examples of per-worker initialization. It's ready for review.

Copy link
Contributor

@PeloWriter PeloWriter left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Almost ready, I'd suggest using an explicit list of supported platforms instead of using notSupported

description: "Learn how to use Sentry's Browser SDK in Web Workers API."
keywords: ["webworkers"]
sidebar_order: 9000
notSupported:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a positive filter with supported: ... so it is only shown for the browser SDK (did we want it anywhere else?)
Because we'll not remember to change this list when adding new JS-based SDKs where this content would not fit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rhcarvalho, I added the supported tag with javascript (for browser, I could not find a specific selector for browser), I'm not sure if that's necessary as this file is under javascript's common content.

Still had to keep notSupported unfortunately to prevent this doc from showing up in SDKs, as they all are falling back to javascript.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yeah the supported filter implies all guides too 😢 .

@PeloWriter do you know of an existing solution for this particular case?

Can we show some content only for JavaScript Browser (and not Angular, Cordova, Ember, React, Vue, etc) without having to list all guides in the notSupported filter? My point was that the not supported list here should be "everything except browser" and it is hard to keep that up-to-date.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly, no workaround yet exists. If the content displays for the primary platform, the only way to turn it off for the associated frameworks is to individually list them as notSupported.

This would be another valuable use of the release registry as a source of truth - it would prevent manual management that is hard to keep up to date.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we don't put this under common/?

There must be a way, in the worst case we can add a new flag includeInGuides: false or similar.
Since this is not intended to be shared, putting it outside of common content would be an easier start.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll consider it's better to ship as is today than to block on this discussion.

Co-authored-by: Rodolfo Carvalho <[email protected]>
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

description: "Learn how to use Sentry's Browser SDK in Web Workers API."
keywords: ["webworkers"]
sidebar_order: 9000
notSupported:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll consider it's better to ship as is today than to block on this discussion.

@rhcarvalho rhcarvalho merged commit 84067a8 into getsentry:master Jun 8, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants