Skip to content

Conversation

@HazAT
Copy link
Member

@HazAT HazAT commented Aug 3, 2018

No description provided.

@HazAT HazAT self-assigned this Aug 3, 2018
@HazAT HazAT requested a review from kamilogorek August 3, 2018 12:15
} else {
dsn = new DSN(this.options.dsn);
// We do nothing in case there is no DSN
return { status: Status.Skipped };
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to provide a reason why we skip some of the events and use logger to let user know about it

const dsn = this.dsnObject;
const auth = {
sentry_key: dsn.user,
sentry_secret: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop it already?

}

/** Returns a string with auth headers in the url to the store endpoint. */
public getStoreEndpoint(urlEncodedHeader: boolean = false): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ughh... can't we just create a new method for this? I really don't like using boolean arguments to functions, as for people not using typescript, it provides no meaning before they dive into source code

const dsn = this.dsnObject;
const header = [`Sentry sentry_version=${SENTRY_API_VERSION}`];
header.push(`sentry_timestamp=${new Date().getTime()}`);
header.push(`sentry_client=${clientName}/${clientVersion}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

here we use sentry_client and above we used sentry_version. We should unify it somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

not really sure what you mean, we need sentry_client for sending a POST.

public getReportDialogEndpoint(dialogOptions: { [key: string]: any }): string {
const dsn = this.dsnObject;
const endpoint = `${this.getBaseUrl()}${dsn.path ? `/${dsn.path}` : ''}/api/embed/error-page/`;
if (dialogOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can skip it and make dialogOptions = {} a default

// Auth is intentionally sent as part of query string (NOT as custom HTTP header)
// to avoid preflight CORS requests
return `${endpoint}?${urlEncode(auth)}`;
this.url = new API(this.options.dsn).getStoreEndpoint();
Copy link
Contributor

Choose a reason for hiding this comment

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

You default to false, but this url actually need headers (another reason to make it a separate method :p)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thx

@codecov-io
Copy link

codecov-io commented Aug 6, 2018

Codecov Report

Merging #1459 into master will decrease coverage by 6.63%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1459      +/-   ##
==========================================
- Coverage   79.24%   72.61%   -6.64%     
==========================================
  Files          50       41       -9     
  Lines        2024     1245     -779     
  Branches      466      237     -229     
==========================================
- Hits         1604      904     -700     
+ Misses        409      331      -78     
+ Partials       11       10       -1
Impacted Files Coverage Δ
packages/core/src/api.ts 100% <100%> (ø)
packages/node/src/transports/base.ts 92.85% <100%> (+3.66%) ⬆️
packages/raven-node/lib/utils.js
packages/raven-node/lib/instrumentation/console.js
packages/raven-node/lib/instrumentation/http.js
packages/raven-node/lib/client.js
packages/raven-node/index.js
packages/raven-node/lib/transports.js
packages/raven-node/vendor/json-stringify-safe.js
packages/raven-node/lib/parsers.js
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb2cdc7...f5a5880. Read the comment docs.

@kamilogorek kamilogorek merged commit 468bc1b into master Aug 28, 2018
@kamilogorek kamilogorek deleted the feature/dsn branch August 28, 2018 08:34
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.

4 participants