Skip to content

Conversation

@maxkosty
Copy link
Contributor

@maxkosty maxkosty commented Feb 23, 2024

Pre-merge checklist

If you work at Sentry, you're able to merge your own PR without review, but please don't unless there's a good reason.

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs
  • PR was reviewed and approved by a member of the Sentry docs team

Description of changes

  • current doc is structured around components of the solution, this restructures it around use-cases: Automatic and Manual
  • remove the other sample beforeSend code that didn't get the slice(-1) fix.
  • [Automatic] changes to the "Once metadata has been injected into modules..." paragraph
  • [Manual] Add a note about importance of using local scope

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Extra resources

@vercel
Copy link

vercel bot commented Feb 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2024 7:09pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
sentry-docs-next ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 7:09pm

@codecov
Copy link

codecov bot commented Feb 23, 2024

Bundle Report

Changes will decrease total bundle size by 6 bytes ⬇️

Bundle name Size Change
sentry-docs-server 5.88MB 0 bytes
sentry-docs-edge-server 3.47kB 0 bytes
sentry-docs-client 5.18MB 6 bytes ⬇️

Copy link
Contributor

@vivianyentran vivianyentran left a comment

Choose a reason for hiding this comment

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

Added some comments but overall LGTM!

@maxkosty maxkosty requested a review from lforst March 7, 2024 03:38
@maxkosty maxkosty marked this pull request as ready for review March 7, 2024 04:01
Copy link
Contributor

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Such a good change!

different release cycles, you may want to identify these or route events to specific projects. This is especially useful if you've set up [module federation](https://module-federation.github.io/) or a similar frontend architecture.

## Identifying the source of errors
Below we offer two approaches. Please note that `Sentry.init()` must be called only once, doing otherwise will result in undefined behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very good to mention. I wonder if we should highlight this even stronger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 37 to 39
First, to identify the source of an error, you must inject metadata that helps identify
which bundles were responsible for the error. You can do this with any of the
Sentry bundler plugins by enabling the `_experiments.moduleMetadata` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to mention here that this should also work for vite and esbuild. I know that we mention the packages above but I am not sure if people will connect the dots. Doesn't have to be in this PR though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added, lmk what you think


```javascript
import * as Sentry from "@sentry/browser";
import { init, makeFetchTransport, moduleMetadataIntegration, makeSimpleMultiplexedTransport, MULTIPLEXED_TRANSPORT_EXTRA_KEY } from "@sentry/browser";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { init, makeFetchTransport, moduleMetadataIntegration, makeSimpleMultiplexedTransport, MULTIPLEXED_TRANSPORT_EXTRA_KEY } from "@sentry/browser";
import { init, makeFetchTransport, moduleMetadataIntegration, MULTIPLEXED_TRANSPORT_EXTRA_KEY } from "@sentry/browser";

Once we added the makeSimpleMultiplexedTransport API we can revisit this but for now I'd leave it away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good.
Reverting sample code to original including this change (as it hasn't been tested)

  • [Automatic] remove redundant if (routeTo.length) { check for empty array (done in matcher)

@lforst We should simplify the API sooner than later though. I feel like current sample code looks convoluted to the point of customers thinking "surely there must be an easier/more elegant way to do it, let me see what those other blog posts that showed up in search say"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, right now it is unfortunately not a priority for us and I'd like to avoid us losing focus but maybe I can sneak your PR in somehow.

@maxkosty
Copy link
Contributor Author

Hi @vivianyentran
there are a few wording changes in this increment 1144e30
would you mind checking my ESL please?

Copy link
Contributor

@vivianyentran vivianyentran left a comment

Choose a reason for hiding this comment

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

Added one more tiny edit. Looks good! Thank you for this update!

Copy link
Contributor

@lforst lforst left a comment

Choose a reason for hiding this comment

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

:shipit:

@maxkosty maxkosty changed the title Simplify/reorganize Micro Frontend Suport (DRAFT) Simplify/reorganize Micro Frontend Suport Mar 23, 2024
@maxkosty maxkosty merged commit 1d406de into master Mar 23, 2024
@maxkosty maxkosty deleted the simpler-micro-frontends branch March 23, 2024 02:18
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
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