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

Reserve promise rejections (i.e. async exceptions) for exceptional circumstances #485

Closed
mattbrunetti opened this Issue Apr 2, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@mattbrunetti

mattbrunetti commented Apr 2, 2017

With the browser's native prompt function, pressing cancel doesn't result in an error, but instead a null return value. (Similarly, with the native alert function, hitting escape doesn't result in an error, and with the native confirm function, clicking no doesn't result in an error but instead a false return value.)

I believe this to be the desired behavior. Especially when using the up-coming async/await feature of ES (being used already via Babel), the current swal2 behavior leads to some awkward control-flow...

let result
let error
try {
  result = await swal({
    text: 'What is your favorite color?'
    input: text,
    showCancelButton: true
  })
} catch (_error) {
  error = _error
}
if (error) {
  swal('I did not get your favorite color because "${error}".')
} else {
  swal(`Your favorite color is ${result}.`)
}

The following would be a bit nicer.

const result = await swal({
  text: 'What is your favorite color?'
  input: text,
  showCancelButton: true
})
if (result.dismiss) {
  swal('I did not get your favorite color because "${result.dismiss}".')
} else {
  swal(`Your favorite color is ${result.value}.`)
}

@limonte Would you review a PR adding an option to enable this behavior? Let's call the option rejections and have it default to true?

@limonte

This comment has been minimized.

Show comment
Hide comment
@limonte

limonte Apr 3, 2017

Member

Please go ahead with a PR 🚀

Member

limonte commented Apr 3, 2017

Please go ahead with a PR 🚀

@toverux

This comment has been minimized.

Show comment
Hide comment
@toverux

toverux Apr 3, 2017

Member

Excellent! Using TypeScript, I've already had this problem.

We should also keep that issue somewhere in our mind for the next major, to enable the new behaviour by default (and maybre remove the option and related code).

Member

toverux commented Apr 3, 2017

Excellent! Using TypeScript, I've already had this problem.

We should also keep that issue somewhere in our mind for the next major, to enable the new behaviour by default (and maybre remove the option and related code).

@mattbrunetti

This comment has been minimized.

Show comment
Hide comment
@mattbrunetti

mattbrunetti Apr 3, 2017

@limonte @toverux Does the same go with the API for the inputValidator method? It should signal a validation error (which is a user error, not a technical/system error) by resolving with the validation message, rather than rejecting with the validation message, right? A falsy resolution value would signal that validation succeeded.

The control-flow is not as bad here, but I think in principal we should not mix user errors with actual technical/system errors.

@limonte @toverux Does the same go with the API for the inputValidator method? It should signal a validation error (which is a user error, not a technical/system error) by resolving with the validation message, rather than rejecting with the validation message, right? A falsy resolution value would signal that validation succeeded.

The control-flow is not as bad here, but I think in principal we should not mix user errors with actual technical/system errors.

@mattbrunetti

This comment has been minimized.

Show comment
Hide comment
@mattbrunetti

mattbrunetti Apr 3, 2017

It makes it difficult to debug actual errors thrown inside the function. Also, if there are any errors in production, I want them bubbled up to Rollbar error reporting.

Same goes for rejections in preConfirm; I think they should be treated as real errors instead of a user validation message.

That said, to support validations for the "multiple-input" scenario, could we always call the function, even if input option was not provided?

It makes it difficult to debug actual errors thrown inside the function. Also, if there are any errors in production, I want them bubbled up to Rollbar error reporting.

Same goes for rejections in preConfirm; I think they should be treated as real errors instead of a user validation message.

That said, to support validations for the "multiple-input" scenario, could we always call the function, even if input option was not provided?

@limonte

This comment has been minimized.

Show comment
Hide comment
@limonte

limonte Apr 7, 2017

Member

Both of your comments make sense. But for beginning please implement the scope of the initial comment only. After reviewing and testing I will be able to decide what to do next. Thanks @mattbrunetti !

Member

limonte commented Apr 7, 2017

Both of your comments make sense. But for beginning please implement the scope of the initial comment only. After reviewing and testing I will be able to decide what to do next. Thanks @mattbrunetti !

@zenflow

This comment has been minimized.

Show comment
Hide comment
@zenflow

zenflow Oct 20, 2017

Collaborator
const {dismiss, value: favoriteColor} = await swal({text: 'What is your favorite color?', input: 'text'})
if (dismiss) {
  swal(`I did not get your favorite color because "${dismiss}".`)
} else {
  swal(`Your favorite color is "${favoriteColor}".`)
}
const {value: name} = await swal({text: 'Full name?', input: 'text'})
const {value: location} = await swal({text: 'Location?', input 'text'})
await swal(`Hi ${name}, from ${location}!`)
Collaborator

zenflow commented Oct 20, 2017

const {dismiss, value: favoriteColor} = await swal({text: 'What is your favorite color?', input: 'text'})
if (dismiss) {
  swal(`I did not get your favorite color because "${dismiss}".`)
} else {
  swal(`Your favorite color is "${favoriteColor}".`)
}
const {value: name} = await swal({text: 'Full name?', input: 'text'})
const {value: location} = await swal({text: 'Location?', input 'text'})
await swal(`Hi ${name}, from ${location}!`)
@limonte

This comment has been minimized.

Show comment
Hide comment
@limonte

limonte Oct 20, 2017

Member

@zenflow Beautiful, readable and maintainable code! Love it!

Member

limonte commented Oct 20, 2017

@zenflow Beautiful, readable and maintainable code! Love it!

@limonte

This comment has been minimized.

Show comment
Hide comment
Member

limonte commented Nov 16, 2017

@khrystyan88

This comment has been minimized.

Show comment
Hide comment
@khrystyan88

khrystyan88 Nov 28, 2017

Uncaught SyntaxError: await is only valid in async function

Uncaught SyntaxError: await is only valid in async function

@toverux

This comment has been minimized.

Show comment
Hide comment
@zenflow

This comment has been minimized.

Show comment
Hide comment
@zenflow

zenflow Nov 28, 2017

Collaborator

Fun fact: With Chrome 62 you can execute the above example code in your console.

image

But otherwise, top-level await is not allowed.

Collaborator

zenflow commented Nov 28, 2017

Fun fact: With Chrome 62 you can execute the above example code in your console.

image

But otherwise, top-level await is not allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment