vlki / refresh-fetch

Wrapper around fetch capable of graceful authentication token refreshing.
MIT License
84 stars 12 forks source link

Requesting server with token is known to be expired #1

Closed svgpp closed 6 years ago

svgpp commented 6 years ago

First fetch is performed unconditionally even if we already know that token was expired and now refreshing token. Better first check presense of refresh request.

vlki commented 6 years ago

Good point! I will look at it during the week. Feel free to prepare pull request yourself though.

vlki commented 6 years ago

So, I am not sure if I understand your problem correctly. What I think is the problem is that we are fetching the API even when we don't have any token and therefore know that it will fail. Correct me if you had a different thing in mind!

To fix the unnecessary fetch, I believe it should be solved by making sure that the fetch you are passing into configureRefreshFetch rejects with error when the token is not present.

In the example from README it means replacing this

const fetchJSONWithToken = (url, options = {}) => {
  const token = retrieveToken()

  let optionsWithToken = options
  if (token != null) {
    optionsWithToken = merge({}, options, {
      headers: {
        Authorization: `Bearer ${token}`
      }
    })
  }

  return fetchJSON(url, optionsWithToken)
}

with something along the lines of this

const fetchJSONWithToken = (url, options = {}) => {
  const token = retrieveToken()

  if (token == null) {
    return Promise.reject(new Error('Cannot fetch without token, login first'))
  }

  const optionsWithToken = merge({}, options, {
    headers: {
      Authorization: `Bearer ${token}`
    }
  })

  return fetchJSON(url, optionsWithToken)
}
svgpp commented 6 years ago

Hi, Not exactly. I mean replacing first fetch in function configureRefreshFetch with something like:

return (url: any, options: Object) => {
  if (refreshingTokenPromise != null) {
   return refreshingTokenPromise
            .then(() => fetch(url, options))
            .catch(() => {
              // If refreshing fails, continue with original error
              throw error
            })
  } else {
    return fetch(url, options)
    ...

It's just draft of course.

vlki commented 6 years ago

Oh, I see! Added in 321977a86069e1a646ff23e5628172dad467f6d5 , published as version 0.5.0.