Skip to content

CHECK_AUTH should return Promise#49

Open
tecfu wants to merge 2 commits into
realworld-apps:masterfrom
tecfu:bugfix-check-auth-promise
Open

CHECK_AUTH should return Promise#49
tecfu wants to merge 2 commits into
realworld-apps:masterfrom
tecfu:bugfix-check-auth-promise

Conversation

@tecfu

@tecfu tecfu commented Jul 30, 2018

Copy link
Copy Markdown
Contributor

Fixes #47

Comment thread src/store/auth.module.js Outdated
context.commit(PURGE_AUTH)
}
return new Promise((resolve, reject) => {
if (JwtService.getToken()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for a refactor into a variable. Seems much more cleaner then

Comment thread src/store/auth.module.js Outdated
.catch(({response}) => {
context.commit(SET_ERROR, response.data.errors)
})
} else {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can omit the else block here.

if (JwtService.getToken()) {
  // ...
  return
}

context.commit(PURGE_AUTH)
resolve()

It is called If-Guard-Clauses and is recommended by a lot of Clean Code enthusiasts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igeligel I love that you raised this issue of early returns. I read http://blog.timoxley.com/post/47041269194/avoid-else-return-early to get a fix on the debate. Useful comments on that article.

Maybe I'm doing it wrong, but my implementation of an early return here would look like:

return new Promise((resolve, reject) => {
      const token = JwtService.getToken()
      if (!token) {
        context.commit(PURGE_AUTH)
        return resolve()
      }

      ApiService.setHeader()
      ApiService
        .get('user')
        .then(({data}) => {
          context.commit(SET_AUTH, data.user)
          resolve(data)
        })
        .catch(({response}) => {
          context.commit(SET_ERROR, response.data.errors)
          resolve()
        })
    })

Not a fan of that at all.

I think scattering the return adds complexity. The benefit of a single column of indentation really doesn't do anything for me here because we don't have nested conditionals anyway.

If anything I see this supporting the "single exit point" argument, because it would be more difficult for new eyes to debug the scattered return.

By comparison, we previously had:

return new Promise((resolve, reject) => {
      const token = JwtService.getToken()
      if (token) {
        ApiService.setHeader()
        ApiService
          .get('user')
          .then(({data}) => {
            context.commit(SET_AUTH, data.user)
            resolve(data)
          })
          .catch(({response}) => {
            context.commit(SET_ERROR, response.data.errors)
            resolve()
          })
      } else { 
        context.commit(PURGE_AUTH)
        resolve()
      }
    })

Would love to read more arguments on this.

@igeligel

Copy link
Copy Markdown
Collaborator

Hey, @tecfu Since I upgraded the code base to the vue-cli 3 now with this commit eeaeb34 you have some merge conflicts. You can solve them by syncing your fork locally with these commands https://help.github.com/articles/syncing-a-fork/ and then merge the master branch into your branch. You need to fix all of the merge conflicts though.

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.

2 participants