-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
test(feedback): Write some test for feedback utils and error returns #12519
Conversation
const timeout = setTimeout(() => reject('Unable to determine if Feedback was correctly sent.'), 5_000); | ||
|
||
const timeout = setTimeout(() => { | ||
reject(new Error('Unable to determine if Feedback was correctly sent.')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This updates the thrown type to be Error
instead of string
so that users can consistently check & optionally render error values.
Now all problems return as Error objects, and can be used like:
try {
await sendFeedback(...);
} catch (error) {
console.log(error.message); // instead of: `'message' in error ? error.message : error`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, will this break for us (+ existing users) that expect the rejection to be a string? I think we use the rejected value directly as the error message in our error toast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the return/error type changes.
const isolationUser = getIsolationScope().getUser(); | ||
if (isolationUser && Object.keys(isolationUser).length) { | ||
return isolationUser; | ||
} | ||
const globalUser = getGlobalScope().getUser(); | ||
return globalUser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing these two scopes was hard to do directly :(
it('should return the empty user if no explicit user is set', () => { | ||
getCurrentScope().setUser(null); | ||
|
||
expect(getUser()).toEqual({ email: undefined, id: undefined, ip_address: undefined, username: undefined }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getUser()
surprisingly has the return type User | undefined
but when nothing is set we're getting a default empty user.
import { getCurrentScope, getGlobalScope, getIsolationScope } from '@sentry/core'; | ||
import type { User } from '@sentry/types'; | ||
|
||
export function getUser(): User | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we do anything similar in other integrations
ca6b0bd
to
d63238a
Compare
size-limit report 📦
|
stale |
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint
) & (yarn test
).