Skip to content

[NEXT] Frontend -> Client#1323

Merged
HazAT merged 6 commits into
nextfrom
ref/next-renameing
May 11, 2018
Merged

[NEXT] Frontend -> Client#1323
HazAT merged 6 commits into
nextfrom
ref/next-renameing

Conversation

@HazAT
Copy link
Copy Markdown
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
/** 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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