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

Simplify code/examples #731

Closed
balazsorban44 opened this issue Sep 30, 2020 · 4 comments · Fixed by #915
Closed

Simplify code/examples #731

balazsorban44 opened this issue Sep 30, 2020 · 4 comments · Fixed by #915
Labels
question Ask how to do something or how something works

Comments

@balazsorban44
Copy link
Member

balazsorban44 commented Sep 30, 2020

I have seen this many places, like this example code:

signIn: async (user, account, profile) => {
  const isAllowedToSignIn = true
  if (isAllowedToSignIn) {
    return Promise.resolve(true)
  } else {
    // Return false to display a default error message
    return Promise.resolve(false)
    // You can also Reject this callback with an Error or with a URL:
    // return Promise.reject(new Error('error message')) // Redirect to error page
    // return Promise.reject('/path/to/redirect')        // Redirect to a URL
  }
}

When defining an async function, the returned value is automatically resolved, meaning one could refactor the above code. (Notice that throwing an Error would probably also feel more natural):

async signIn(user, account, profile) {
  const isAllowedToSignIn = true
  if (isAllowedToSignIn) {
    return true
  } else {
    // Return false to display a default error message
    return false
    // You can also Reject this callback with an Error or with a URL:
    // throw new Error('error message')) // Redirect to error page
    // return Promise.reject('/path/to/redirect')        // Redirect to a URL
  }
}

This may be only a personal preference, but I think it would also be easier to parse the code for newcomers. With the rise of "async/await", maybe there is lesser need of using Promises all-together. Any special reason I am unaware for using Promise.resolve() everywhere?

@balazsorban44 balazsorban44 added the question Ask how to do something or how something works label Sep 30, 2020
@balazsorban44 balazsorban44 changed the title Simplify code Simplify code/examples Sep 30, 2020
@iaincollins
Copy link
Member

iaincollins commented Oct 1, 2020

Any special reason I am unaware for using Promise.resolve() everywhere?

Good question! This has been asked before and yes, actually it is deliberate.

Internally, we try and avoid this in the library (and there are linter rules configured to discourage this) but the intent is that it's helpful re-enforcing to people that these functions are async and you can consequently do interesting things inside them.

The virtue of doing this is debatable, but a primary driver of the documentation is to reduce the amount of support overhead by making it easier for folks to see how they can solve problems they have - as support is where the majority of time is spent by contributors on this project at the moment.

@balazsorban44
Copy link
Member Author

balazsorban44 commented Dec 1, 2020

I would argue, that #897 just proved that this encourages using callbacks over async/await, which created the issue there, and made the user's code much more complicated than it needed to be (looks like to me that they were thinking they have to use Promise.#{reject,resolve} for it to work)

UPDATE.

The above mentioned issue's author just confirmed it in their last comment that Promise was misleading in the docs as a fairly new developer. I really recommend this to be visited and examples/code be simplified down to just async/await and simple returns without Promise.

@github-actions
Copy link

🎉 This issue has been resolved in version 3.2.0-canary.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Feb 1, 2021

🎉 This issue has been resolved in version 3.3.0-canary.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Ask how to do something or how something works
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants