Initial implementation #1

dodsky · 2017-02-14T17:12:22Z

No description provided.

…ctions

vedrani

I commented this file a lot, but please use it only as suggestion. I think you did a great job :) I didn't review tests, I will look it in second iteration. 🥇

vedrani · 2017-02-15T09:14:48Z

src/helpers/tokenFormatter.js

+  }
+
+  return token;
+}


This should maybe be default way of handling authorization, but we should also allow to override it in config from Builder.

vedrani · 2017-02-15T09:15:40Z

src/helpers/tokenFormatter.js

@@ -0,0 +1,25 @@
+export function formatBearer(token) {


This is only for tests? If so move it to /test/helpers

vedrani · 2017-02-15T09:16:32Z

src/const.js

+export const ENVIRONMENT_IS_REACT_NATIVE = typeof navigator === 'object' && navigator.product === 'ReactNative';
+export const ENVIRONMENT_IS_NODE = typeof process === 'object' && typeof require === 'function';
+export const ENVIRONMENT_IS_WEB = typeof window === 'object';
+export const ENVIRONMENT_IS_WORKER = typeof importScripts === 'function';


This should be changed into functions with camelCase and moved maybe to service/environment.js

vedrani · 2017-02-15T09:19:21Z

src/index.js

+      return fetchInterceptor(fetch, ...args);
+    };
+  })(env.fetch);
+}


This function with above check of environment should be encapsulated into environment that would be called in index.js like environment.init(); only once upon index.js load. Names are just suggestions.

vedrani · 2017-02-15T09:20:23Z

src/index.js

+  refreshToken: null,
+};
+
+let refreshAccessTokenPromise = null;


All of above let variables should be changed into const and moved into encapsulated parts of lib that use them.

vedrani · 2017-02-15T12:37:08Z

src/index.js

+/**
+ * Configures fetch token intercept
+ */
+export function configure(initConfig) {


Interceptor class and Interceptor class should pass part of config to AccessToken provider?

I'm currently passing the whole config to AccessTokenProvider until we settle on the design. It's easy to extract necessary methods in the end.

vedrani · 2017-02-15T12:38:01Z

src/index.js

+  STATUS_OK,
+} from './const';
+
+let config = {


Document each config key

vedrani · 2017-02-15T12:39:42Z

src/index.js

+
+          return response;
+        })
+        .then(response => {


Please check usage of .then sometimes it's not necessary, I think once you encapsulate logic into methods that need for .then will diminish. You explained me concept of config methods that returns promise, and for that cases you don't have other options.

This whole part was redesigned into requestUnit idea as part of fetchInterceptor class and fetchWithRetry method.

vedrani · 2017-02-15T12:41:43Z

src/index.js

+    if (!tokens.accessToken) {
+      // check if we are alredy fetching it
+      if (!refreshAccessTokenPromise) {
+        // as a side-effect refreshTokenPromise is set


Don't like side-effect :)

Sideeffect is now part of accessTokenProvider :)

vedrani · 2017-02-15T12:42:47Z

src/index.js

+            outerReject(error);
+          } else {
+            // if we fail to resolve refresh token, and it's not internal error
+            // just pass the request normally


This part is weird, I wouldn't expect it.

Here's how normal fetch behaves:
fetch(url).then(result).catch(error)

catch is called only on network errors (e.g. DNS unresolved)

then is called on all other responses, including 401

Initially I've solved this flow by throwing this exception, then capturing it in main fetch promise and resolving accordingly. In new version I did it without throwing exceptions.

dodsky · 2017-02-16T08:39:43Z

These are all great suggestions, I've already fixed some od these suggestions on the following commit, like encapsulating refresh token handling into separate class, and your ideja od interceptor correlates with my idea od interceptor provider. Also to enable promisifying any method od config I've converted a request into a smarter object which I've called 'requestUnit' which passes through interceptor promise pipeline and holds resolved values of all config functions. This effectively enables us to cashe values for resolved config methods and whole process acts as a middleware chain (got the ideja from node). :)

* fetchInterceptor - main class for intercepting * accessTokenProvider - access token resolver multiple services to increase readability as per PR comments

Fixed scope for fetch method

vedrani

We should discuss some aspects of code.

vedrani · 2017-02-21T09:34:36Z

test/helpers/tokenFormatter.spec.js

+      expect(result).to.be.equal('token');
+    });
+  })
+});


vedrani · 2017-02-21T09:36:04Z

package.json

+    "access"
+  ],
+  "author": "Shoutem",
+  "license": "MIT",


BSD? custom?

Note, this should probably be updated on redux io repo too.

vedrani · 2017-02-21T09:36:22Z

package.json

+  "author": "Shoutem",
+  "license": "MIT",
+  "bugs": {
+    "url": ""


link to github issues page?

vedrani · 2017-02-21T09:37:06Z

src/accessTokenProvider.js

+  isResponseUnauthorized,
+} from './services/http';
+
+export class AccessTokenProvider {


Add nice jsdoc comment /** about this class, explaining it to others

vedrani · 2017-02-21T09:45:22Z

src/accessTokenProvider.js

+  refresh() {
+    // if token resolver is not authorized it should just resolve
+    if (!this.isAuthorized()) {
+      return Promise.resolve();


Should it reject?

If it rejects it will cause a chain reaction which will cause fetchIntercept chain to reject also, which in turn will reject an initial fetch request which is undesirable as we don't want to break the fetch spec which currently states that fetch will only reject on network errors and this is not a network error.

But indeed it could be potentially problematic if you call renew without calling authorize first as it will silently ignore your renew requests. Two solutions here: silently fail with console error, or raise an exception which will be caught by fetchInterceptor which won't propagate it out of fetch call, but catch it, perhaps again log it, and then resolve as normal request would. I feel that the second solution only raises the handling to upper level without any flow control benefits as this can't be handled.

Looking from outside, I would want for my requests to fail with 40x error with equal behavior as in case that I don't use fetch-token-intercept and didn't add proper Auth header. Also it would be useful to see additional error or warning from fetch-token-intercept about need to call authorize with instruction to pass refreshToken.

vedrani · 2017-02-21T13:22:05Z

src/fetchInterceptor.js

+      .then(this.fetchRequest)
+      .then(this.shouldInvalidateAccessToken)
+      .then(this.invalidateAccessToken)
+      .then(this.handleUnauthorizedRequest)


handleUnauthorizedRequest hides that you are renewing token. Maybe this could be more explicit, by rejecting and then in rejectHandler try to resolve problem?

vedrani · 2017-02-21T13:26:13Z

src/fetchInterceptor.js

+          this.config.onResponse(response);
+        }
+
+        outerResolve(response);


This code in then block could be in handleResponse()?

The only reason that it isn't is because it needs access to methods for resolving fetch promise (outerResolve and outerReject are basically mapped like this fetch.then(outerResolve).then(outerReject)), but when I though about it, it totally makes sense to put references of outer fetch reject and resolve in request unit as any stage of pipeline could do

vedrani · 2017-02-21T13:28:31Z

src/fetchInterceptor.js

+            shouldFetch: !!accessToken,
+          }))
+          .then(this.authorizeRequest)
+          .then(this.fetchRequest)


This block in if (isResponseUnauthorized(response)) { is problematic, what happens if it fails twice? You only have one retry right? Should you reuse fetchWithRetry and move request through flow once again?

vedrani · 2017-02-21T13:30:53Z

src/helpers/tokenFormatter.js

+  }
+
+  const bearerRegex = /^Bearer (.+)$/;
+  const matches = bearerRegex.exec(authorizationHeaderValue);


string.match(regex)

vedrani · 2017-02-21T13:35:22Z

test/fetchInterceptor.spec.js

+        });
+    });
+
+    it('should propagate 401 for multiple requests', function (done) {


What happens here? Can we test what happens if one request gets multiple 401s?

Request is emitted with expired token

Response is 401

token is refreshed, request is reemitted

Response is 401

token is refreshed, request is reemitted

Response is 401

token is refreshed, request is reemitted
...

Wrote a test, it will retry as many times as it is defined on config with fetchRetryCount property. (should stop after retry count is exceeded and resolve unauthorized)

Bump versoin

Refactored tests Minor renames and improvements

vedrani

There is a couple of minor changes.

vedrani · 2017-02-27T08:24:36Z

package.json

@@ -0,0 +1,59 @@
+{


You don't ahve README file, add it. I know you don't have time to write everything. But maybe you could just add short description about what this lib solves and add link to somethnig like this:
https://auth0.com/blog/refresh-tokens-what-are-they-and-when-to-use-them/

vedrani · 2017-02-27T08:27:33Z

src/AccessTokenProvider.js

+  renew() {
+    // if token resolver is not authorized it should just resolve
+    if (!this.isAuthorized()) {
+      console.log('Please authorize provider before renewing or check shouldIntercept config.');


vedrani · 2017-02-27T08:28:50Z

src/AccessTokenProvider.js

+   * @param refreshToken
+   * @param accessToken
+   */
+  authorize(refreshToken, accessToken) {


Do you need to pass accessToken here?

I want to allow passing both refresh token and access token for two reasons:

perhaps you already have both stored externally and both are valid (why forcing a renew?)

RFC spec allows authorization response to return both https://tools.ietf.org/html/rfc6749#page-10

vedrani · 2017-02-27T08:30:02Z

src/AccessTokenProvider.js

+  }
+
+  handleAccessToken(token, resolve) {
+    this.tokens.accessToken = token;


this.tokens = {...this.tokens, accessToken}

vedrani · 2017-02-27T08:30:11Z

src/AccessTokenProvider.js

+    return this.config.parseAccessToken(response);
+  }
+
+  handleAccessToken(token, resolve) {


accessToken

vedrani · 2017-02-27T08:40:14Z

src/FetchInterceptor.js

+  invalidateAccessToken(requestUnit) {
+    const { shouldIntercept, shouldInvalidateAccessToken } = requestUnit;
+
+    if (shouldIntercept && shouldInvalidateAccessToken) {


vedrani · 2017-02-27T08:42:30Z

src/FetchInterceptor.js

+    }
+
+    if (this.config.onResponse) {
+      this.config.onResponse(response);


What if someone in onResponse function doesn't know how to handle response with clone is it going to destroy it before fetchResolve, should you pass response.clone here?

As this is the last callback it won't matter, but generally speaking, when accessing body you should clone first, because if anyone also has to read it after you it will fail I don't clone it first to avoid clone overhead when it's not necessary. Also, I've designed onResponse as fire and forget as we don't wait for events to finish during fetch intercept.

vedrani · 2017-02-27T08:45:48Z

src/FetchInterceptor.js

+    throw new Error(error);
+  }
+
+  isConfigValid() {


maybe put it higher in class to be closer to call

vedrani · 2017-02-27T08:47:42Z

src/services/RetryCountExceededException.js

+  this.message = 'Retry count has been exceeded';
+  this.requestUnit = requestUnit;
+
+  // Use V8's native method if available, otherwise fallback


What we gain with this? It only for Chrome?

http://stackoverflow.com/a/27724419/1008529
...with the enhancement that stack traces work on Firefox and other browsers.

vedrani · 2017-02-27T08:48:25Z

src/services/RetryCountExceededException.js

+
+RetryCountExceededException.prototype = Object.create(Error.prototype);
+RetryCountExceededException.prototype.name = "RetryCountExceededException";
+RetryCountExceededException.prototype.constructor = RetryCountExceededException;


new line.
Is there a ES6 way to do this? using extends

Babel 6.x does not correctly support extending core classes yet:
http://stackoverflow.com/questions/31089801/extending-error-in-javascript-with-es6-syntax
babel/babel#3083

there's a babel plugin https://www.npmjs.com/package/babel-plugin-transform-builtin-extend that enables extending from Error and Array. Don't know how we feel about this..

I would add this babel plugin, It's only overhead during transpiling but simplifies code.

Added readme Minor renames in AccessTokenProvider and FetchInterceptor Version bumped

zrumenjak

Looks awesome 👍

zrumenjak · 2017-03-17T19:40:44Z

README.md

+  //Event invoked when access token has changed
+  onAccessTokenChange: null,
+
+  //Event invoked when response is resolved


You usually have a single space after // except in the examples above :)

zrumenjak · 2017-03-17T20:01:00Z

README.md

+}
+```
+
+All required methods support returning a promise to enable reading of body.


Maybe ... return a promise to allow reading of the request body.

zrumenjak · 2017-03-17T20:03:51Z

src/AccessTokenProvider.js

+  }
+
+  /**
+   * Returns current authorization for fetch fetchInterceptor


fetch fetchInterceptor is a bit confusing here...

zrumenjak · 2017-03-17T20:07:58Z

src/FetchInterceptor.js

+  }
+
+  /**
+   * Authorizes fetch interceptor with given renew token


zrumenjak · 2017-03-17T20:09:38Z

src/FetchInterceptor.js

+  resolveIntercept(resolve, reject, ...args) {
+    const request = new Request(...args);
+    const { accessToken } = this.accessTokenProvider.getAuthorization();
+    const requestUnit = this.createRequestUnit(request, resolve, reject);


What is a request unit? Maybe add a short description to explain it here.

Renamed to RequestContext, also added comments on createRequestContext method describing what it represents.

zrumenjak · 2017-03-17T20:18:06Z

src/FetchInterceptor.js

+      .catch(this.handleUnauthorizedRequest);
+  }
+
+  createRequestUnit(request, fetchResolve, fetchReject) {


It wasn't clear to me what a request unit should be from its name. Maybe we should consider renaming it to something like RequestContext or possibly RequestTask.

dodsky added 5 commits February 9, 2017 13:00

Initial implementation

b907192

Refactoring after initial usage, added more test coverage

211c01f

Added more tests, revised some configuration options

8f25d14

Refactored config to enable promises as return values from config fun…

9fb6c05

…ctions

Added license, fixed version

7a4236b

dodsky self-assigned this Feb 14, 2017

dodsky requested a review from vedrani February 14, 2017 17:12

vedrani suggested changes Feb 15, 2017

View reviewed changes

Refactored initial solution into multiple files, including:

79ab1ad

* fetchInterceptor - main class for intercepting * accessTokenProvider - access token resolver multiple services to increase readability as per PR comments

dodsky requested a review from vedrani February 16, 2017 22:54

dodsky added 3 commits February 17, 2017 00:51

Minor renames

af42f0e

Added shouldFetch and onResponse

2d563b4

Fixed scope for fetch method

Version bump

952e1c2

vedrani suggested changes Feb 21, 2017

View reviewed changes

dodsky added 2 commits February 21, 2017 15:34

Fix invalid imports

35ba205

Bump versoin

Refactored flow logic with custom exceptions

6598de8

Refactored tests Minor renames and improvements

dodsky requested a review from vedrani February 23, 2017 23:53

dodsky added 2 commits February 24, 2017 11:21

Version bump

458ec47

Fixing invalid exception handling

3637777

vedrani approved these changes Feb 27, 2017

View reviewed changes

vedrani requested a review from zrumenjak February 27, 2017 08:50

Refactored exceptions to be more es6-like

8f8357b

Added readme Minor renames in AccessTokenProvider and FetchInterceptor Version bumped

dodsky merged commit 0921a59 into master Feb 27, 2017

zrumenjak reviewed Mar 17, 2017

View reviewed changes

dodsky deleted the initial-implementation branch April 5, 2017 13:12

Initial implementation #1

Initial implementation #1

Conversation

dodsky commented Feb 14, 2017

vedrani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dodsky Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dodsky commented Feb 16, 2017

vedrani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dodsky Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

vedrani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zrumenjak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dodsky Feb 16, 2017 •

edited

Loading

dodsky Feb 23, 2017 •

edited

Loading