Skip to content

fixed username to name in RavenUserContext (in .d.ts) #820

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

LucaVazz
Copy link
Contributor

In the d.ts for raven.js the interface RavenUserContext has the member `username.

If you look at the corresponding js-file you see that the meber should really be called name.

In the d.ts for raven.js the interface `RavenUserContext ` has the member `username.

If you look at the corresponding [js-file](https://github.com/getsentry/raven-js/blob/master/src/raven.js#L658) you see that the meber should really be called `name.`
@benvinegar
Copy link
Contributor

The line you're pointing to is for showReportDialog. It takes different parameters than the user context, which does indeed take username.

Basically, RavenUserContext should not be used for showReportDialog. I'll make a new PR since people are waiting for a point release.

@LucaVazz
Copy link
Contributor Author

You are right, wired that I somehow didn't catch that RavenUserContext is also used for the "general" Context-Information about the user...

While we are at it: I also just noticed that RavenUserContext lacks the ip_address-member.

@benvinegar
Copy link
Contributor

@LucaVazz - RavenUserContext is also supposed to accept arbitrary key/values which are respected in the Sentry UI – any idea how to to declare that?

Aside: I definitely accepted that PR way too early.

@LucaVazz
Copy link
Contributor Author

I think L31-33 shows the correct way to declare the extra Key/Value-Interface.

Although I don't understand, why @connor4312 did not declare it the same way a few lines down for extra... Maybe he can clarify this?

@LucaVazz
Copy link
Contributor Author

LucaVazz commented Jan 12, 2017

And yet another minor thing I noticed while reading through: RavenGlobalOptions and RavenOptionsboth have a memberstacktrace. It should therefore be moved into CommonRavenOptions`.

Not related to raven.d.ts: It is correct with respect to the actual js-source, that the global options except a member named serverName while the "local" options when sending an error except server_name. (See here) But why? Is this really intended or a Bug?

@connor4312
Copy link
Contributor

The extra event was originally typed to any, I didn't change it was I wasn't sure if it was able to take other data types. If it does, awesome! Stricter typings are always great.

@LucaVazz
Copy link
Contributor Author

@benvinegar Can you clarify this, I'm also not sure if extra can take anything or just key/value-pairs?

@benvinegar
Copy link
Contributor

extra can take nested values, e.g. arrays and objects.

@LucaVazz
Copy link
Contributor Author

okay, then any is the right chocie, as is noted in the TypeScript Documentation about .d.ts-Files:

If you’re tempted to use the type Object, consider using any instead. There is currently no way in TypeScript to specify an object that is “not a primitive”.

@connor4312
Copy link
Contributor

It might make sense to have it take { [prop: string]: any } to enforce that the top-level type is a plainish object.

@LucaVazz
Copy link
Contributor Author

Yeah, that should do it. As you said, it should be typed as strict as possible.

@benvinegar
Copy link
Contributor

Any chance of an update to this PR (or a new one)? I'm not a TypeScript developer, so I'm concerned I'd get this wrong.

@LucaVazz
Copy link
Contributor Author

I have an update nearly finished. Would you rather have it appended to this PR or as a new one?

@benvinegar
Copy link
Contributor

Yeah maybe a new one so we can keep the history here with the original context / original PR.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants