-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Add makeSimpleMultiplexedTransport
#10798
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
Hoping to reduce complexity for setting up micro-frontends routing.
It is a nice simplification!
Rather than add another method, maybe we could just set this as a default for matcher?
Rather than add another method, maybe we could just set this as a default for
matcher?
Agreed, good point
makeSimpleMultiplexedTransport (追記ここまで)
size-limit report 📦
|
makeSimpleMultiplexedTransport (削除ここまで)makeSimpleMultiplexedTransport (追記ここまで)
@realkosty I think a separate function is more clear for now in terms of functionality. I don't think we should overload the multiplexedTransport with functionality.
I cleaned your draft up a bit and added tests. Let me know if this feels like a good API for you - I feel like you have the most experience with implementing it in the wild. Imo this is good to ship otherwise. We should probably also backport this to v7 since this PR is now pointing to v8.
* Simplify/reorganize Micro Frontend Suport * Update micro-frontend-support.mdx * default matcher instead of a new method See: getsentry/sentry-javascript#10798 (comment) * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> * Update micro-frontend-support.mdx * Note about calling Sentry.init() only once * Address Luca's comments in micro-frontend-support.mdx * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: Luca Forstner <luca.forstner@sentry.io> * Update docs/platforms/javascript/common/configuration/micro-frontend-support.mdx Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> --------- Co-authored-by: vivianyentran <20403606+vivianyentran@users.noreply.github.com> Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
I think the naming of the function sucks but I will merge this in. We can rename or alias later.
actually the naming sucks too much for me to comfortably approve this
@lforst what do you want to do about the naming of the function? Any suggestions for renaming?
Well find a better name. Currently I don't have ideas.
nice
Can we revisit this, and instead of creating a new transport (and figuring out the naming), just make the matcher argument for makeMultiplexedTransport optional, using the implementation from this PR by default?
So users could do:
{ transport: makeMultiplexedTransport(makeFetchTransport) }
to get this outcome? cc @realkosty & @andreiborza
@mydea sounds reasonable to me
Uh oh!
There was an error while loading. Please reload this page.
Hoping to reduce complexity for setting up micro-frontends routing.
Also: I suppose we could just make Matcher argument optional instead of exporting a separate function... that may be a cleaner design