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

Change close/flushes to return false on failure #3823

Closed
wants to merge 1 commit into from
Closed

Change close/flushes to return false on failure #3823

wants to merge 1 commit into from

Conversation

cnorthwood
Copy link

This matches the comment, and currently rejecting a promise will cause an uncaught promise exception which doesn't get caught and the causes the application to crash. Additionally, rejecting a promise with a boolean doesn't cause a stack trace which made this hard to diagnose. If keeping the rejection is preferred, then it should instead reject with an error object, not just a boolean.

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

This matches the comment, and currently rejecting a promise will cause an uncaught promise exception which doesn't get caught and the causes the application to crash. Additionally, rejecting a promise with a boolean doesn't cause a stack trace which made this hard to diagnose. If keeping the rejection is preferred, then it should instead reject with an error object, not just a boolean.
@kamilogorek
Copy link
Contributor

Personally agree that it should be return false, as there was no "real" error. We just bailed due to client not being available, and it's not destructive action. We could eventually add:

logger.error(`Error while flushing events: no client available`);

just before the return itself.

This change should be backwards compatible, as attached rejection handlers will still convert inside client.flush calls. The only thing that can change, is if someone logged the rejection themselves, eg:

Sentry.flush(2000).catch(e => {
  console.error('Unable to flush')
})

however, in this case as you mentioned e is useless, so I'm fine "breaking" this scenario, as again, it's not destructive.

This should also be changed in browser/sdk.ts and JSDoc should be updated to include If you provide a timeout and the queue takes longer to drain *or the client is not initialized*, the promise returns false..

ref: #3031

cc @AbhiPrasad @lobsterkatie

@lobsterkatie
Copy link
Member

@cnorthwood - Would you like to make Kamil's suggested changes here, or shall I open a separate PR?

@cnorthwood
Copy link
Author

@lobsterkatie if you want to have a look that'd be great! It's still on my to-do list to have a look at this but don't let me block it

@lobsterkatie
Copy link
Member

lobsterkatie commented Jul 27, 2021

Okay - I've replicated your work (and Kamil's suggestions) in #3846, so I'm going to close this. Thanks for the contribution, though!

@cnorthwood
Copy link
Author

Thank you for picking this up @lobsterkatie :)

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