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

[NEXT] Frontend -> Client #1323

Merged
merged 6 commits into from
May 11, 2018
Merged

[NEXT] Frontend -> Client #1323

merged 6 commits into from
May 11, 2018

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented May 9, 2018

No description provided.

@HazAT HazAT self-assigned this May 9, 2018
@HazAT HazAT requested a review from kamilogorek as a code owner May 9, 2018 12:26
@@ -12,7 +12,9 @@ const sendRavenEvent = Raven._sendProcessedPayload.bind(Raven) as SendMethod;

/** Normalizes the event so it is consistent with our domain interface. */
function normalizeRavenEvent(event: SentryEvent): SentryEvent {
const ex = (event.exception || {}) as { values?: SentryException[] };
const ex = ((event && event.exception) || {}) as {
Copy link
Member Author

@HazAT HazAT May 9, 2018

Choose a reason for hiding this comment

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

There was an issue when running it with SDK loader thatevent was null/undefined.

Copy link
Member

Choose a reason for hiding this comment

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

That's not to be solved here.

Copy link
Member

Choose a reason for hiding this comment

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

Can we fix this in captureEvent and change the top-level signature to SentryEvent | undefined? This is way too deep and also defies type checking.

@HazAT HazAT changed the title [WIP] Frontend -> Client [WIP] [NEXT] Frontend -> Client May 9, 2018
public constructor(client: Client<BrowserOptions>) {
this.client = client;
}
public constructor(private readonly options: BrowserOptions) {}
Copy link
Member

Choose a reason for hiding this comment

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

We probably also want the DSN here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that we only pass in the options and if we need some fancy behavior our DSN class supports, we create a DSN object in the backend.

See: https://github.com/getsentry/raven-js/pull/1323/files#diff-261a7b228c5d644e74c94c3f99ba8a9eR147

Copy link
Member

Choose a reason for hiding this comment

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

WFM. We can assume that the DSN must be valid at that point, so there's also no need to catch exceptions anymore.

@HazAT HazAT changed the title [WIP] [NEXT] Frontend -> Client [NEXT] Frontend -> Client May 11, 2018
Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

LGTM!

@HazAT HazAT merged commit 23d87ed into next May 11, 2018
@HazAT HazAT deleted the ref/next-renameing branch May 11, 2018 09:18
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.

3 participants