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

unified fixes #148

Merged
merged 8 commits into from
Oct 28, 2020
Merged

unified fixes #148

merged 8 commits into from
Oct 28, 2020

Conversation

rxlabz
Copy link
Contributor

@rxlabz rxlabz commented Oct 28, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

@rxlabz rxlabz changed the title remove an obsolete comment unified fixes Oct 28, 2020
@@ -22,7 +22,13 @@ abstract class SentryClient {
SentryClient.base(this._options, {String origin}) {
_random = _options.sampleRate == null ? null : Random();
if (_options.transport is NoOpTransport) {
_options.transport = HttpTransport(options: _options, origin: origin);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marandaneto shouldn't we catch the potential Dns.parse exception ? What do you think would be the best strategy or preferred wording here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rxlabz rxlabz marked this pull request as ready for review October 28, 2020 11:21
@rxlabz rxlabz requested a review from bruno-garcia as a code owner October 28, 2020 11:21
@rxlabz rxlabz requested a review from marandaneto October 28, 2020 11:28
// origin is necessary for sentry to resolve stacktrace
return SentryBrowserClient._(options);
}
factory SentryBrowserClient(SentryOptions options) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

make me wonder if now we need to have a SentryBrowserClient and IOClient only because of the origin field.

Copy link
Contributor Author

@rxlabz rxlabz Oct 28, 2020

Choose a reason for hiding this comment

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

true, we could move the origin settings in transport, and use the same strategy as for gzip compression => it will remove the need for 2 clients

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM

@rxlabz rxlabz merged commit 108ac6c into feature/unified-api Oct 28, 2020
@rxlabz rxlabz deleted the fix/unified-fixes branch October 28, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants