Jquery adapter refactor #76

AtofStryker · 2020-08-06T20:55:56Z

This is a revisit of the RFC and #64 and #65, a PR which is similar to this one, with some changes to the original proposal. I apologize for the gnarly diff in advance. I highly recommend stepping through the commit history to see what changed and when as it will be easier to track.

Strategy

The HttpAdapter has been broken out into multiple classes. These classes include the JQueryAjaxHttpAdapter and whatever other adapter we decide to implement. This is the PR to set up the structure to implement other types of adapters.

Since these adapters share common functionality, common methods have been extracted into a base HttpAdapter class to provide base functionality without duplication. This allows the JQueryAjaxHttpAdapter and other adapters to implement only a few methods that are specific only to their underlying technological implementations while sharing the same public API.

An example of the implementation is below. It should function the same as the previous HttpAdapter

import {
  Store,
  JQueryAjaxHttpAdapter
} from '@groveco/backbone.store';

const adapter = new JQueryAjaxHttpAdapter({

  urlPrefix: '/my-prefix',

  requestInterceptor(config) {

   // edit config

   return config

  },
  responseInterceptor(response) {

   // do something with response

   return response

  },

});

const store = new Store(adapter);

An example of direct request consumption is below:

try {

  const store = getStoreInstance()

  const args = await store.request('/api/login/', {

    method: 'GET' | 'POST' | 'PUT' | 'DELETE',

    headers: {

      'Content-Type': 'application/json'

    },

    data: {

      prop1,

      prop2

    }

  });

  // Do something with the successful request

} catch (e) {

  // Do something with the failed request

}

Rationale

External API

`request` method added to public API via `Store` -> `HttpAdapter`

To allow for support to make generic requests that do not use the store indirectly, the decision to add a public request method was invoked. This allows for users to make requests directly through the Store The Store proxies to the HttpAdapter, allowing for a generic method to make any request a user sees fit.

`requestInterceptor` callback added

Consuming applications have a need to intercept requests before being sent. The most common use case for this is to add headers, but other options can be utilized here. Initially, this function was going to be called OnBeforeRequest. Due to consumer needs, we need to continue a form of interceptor behavior. This turns over configuration editing to consuming application, which is up to the discretion of the consumer to use responsibly.

`responseInterceptor` callback added

Consuming applications have a need to intercept response before being processed. The most common use case for this is to clean cache or perform a redirect in the case of undesirable http status codes.

Internal API

Moving `serializeRequest` and `buildUrl` to base class

Moving serializeRequest functionality to its own function within a base class allows for the same logic to be shared by multiple adapters. This makes the logic easier to test and helps guarantee the behavior is the same between adapters. Same goes for buildUrl.

Adding `isInternal` argument to `_makeRequest`

Due to some existing conditions around potential dataType bugs within jQuery's ajax handler, isInternal was added to the method signature of _makeRequest to keep existing behavior consistent. This change might not be necessary, but wanted to allow for the least change possible within the implementation to prevent potential issues. Interceptors will also be executed when isInternal is set to true, as one off requests may/may not want to be run in those executors.

Default `application/vnd.api+json` header arguments in `_http`

Adding application/vnd.api+json to the default implementation of _http allows for all pre existing calls to _http to behave the same, ensuring existing legacy behavior.

Since this is a default argument, we leverage request to override the default headers implementation within _http to allow for generic requests without predefined behavior.

Common FAQ

Is the public API the same?

In short, they can be. Methods have not been removed and all methods should function the same. The only public API options added are request and contructor options for headers and addHeadersBeforeRequest

Since there was significant refactor within the jQueryAjaxHttpAdapter, there is some risk involved that the default implementation did not break anything. We may want to implement a custom error class for the jQueryAjaxHttpAdapter to make tracing Sentry easier in case problems arise.

Are Backbone Collections behind the scenes using our default headers

In short, yes. Regardless of adapter and to be backwards compatible with backbone, ajaxSetup will need to be used to attach these headers. The requestInterceptor function can likely be used just like the beforeSend in ajaxSetup

Will the new `request` method use the interceptors?

Only if the internal key is set to true in options

tests/http-adapter.spec.js

AtofStryker · 2020-08-07T17:18:34Z

src/http-adapter.js

-    return this._ajax('DELETE', link)
-  }
-
-  _ajax (type, url, data) {


This method has been broken up into _http and _checkSerializeRequests, with some of the none abstractions moved to _makeRequest that are implementation specific

AtofStryker · 2020-08-07T17:21:03Z

src/http-adapter.js

-
-    // Stringify data before any async stuff, just in case it's accidentally a mutable object (e.g.
-    // some instrumented Vue data)
-    if (data && ['PATCH', 'POST'].indexOf(type) > -1) {


90-92 moved to _makeRequest in the jQueryHttpAdapter

AtofStryker · 2020-08-07T17:22:47Z

src/http-adapter.js

-      data = JSON.stringify(data)
-    }
-
-    let promise


94-112 moved to _checkSerializeRequests in the base HttpAdapter

AtofStryker · 2020-08-07T17:23:48Z

tests/store.spec.js

@@ -522,4 +522,25 @@ describe('Store', function () {
    it.todo('fetches a single resource')
    it.todo('updates the resource with the response')
  })
+
+  describe('request', function () {


this can be moved to a separate PR if needed to reduce size

Seems manageable to me. These tests seem good to include.

racoleman

Nice work. Since we're changing all this code anyway, how about running it through eslint --fix?

src/http-adapter/index.js

racoleman · 2020-08-11T16:07:39Z

src/http-adapter/index.js

+    this.serializeRequests = false
+    this._outstandingRequests = new Set()
+    this.requestInterceptor = options.requestInterceptor || (() => {})
+    this.responseInterceptor = options.responseInterceptor || (() => {})


You could potentially make these defaults class fields; they were added to preset-env in 7.10. Of course, it would break the compiled version of backbone.store unless you update it to Babel 7 first.

We could do that. We might want to rebase and merge #73 if that is the route we go, as well as add documentation that the source is being consumed raw

src/http-adapter/index.js

racoleman · 2020-08-11T18:51:36Z

src/http-adapter/index.js

+   * @param {String} url - the url of the requwst.
+   * @param {Object} [data] - a data payload represented as a key/value pair.
+   * @param {string} [headers={'Accept': 'application/vnd.api+json', 'Content-Type': 'application/vnd.api+json'}] - headers to be included with the request
+   * @param {string} [isInternal=true] - Whether the request is going to an owned/internal server, in which case header options


I'm not sure I fully understand the need for both headers and isInternal. Even if both are necessary, it might be more transparent to have more specific params that describe exactly what they do, rather than a vague multifunctional param.

Agreed. I think both are necessary since isInternal allows for execution of the request/response interceptors (unless we want to invert that control). Maybe we should, at a minimum, document the isInternal option in more detail?

After reading over it again, I think isInternal is unnecessary and potentially confusing. If I pass a request/response interceptor to the adapter, I would expect it to be used. Perhaps remove the empty function defaults and call them if they're defined?

Agreed. I find it to be confusing. Likely will use requestInterceptors as an option, but will keep the default functions on the interceptor as it is just easier to handle and likely more type safe

racoleman · 2020-08-11T21:30:08Z

src/http-adapter/jquery/index.js

+          if (isInternal) {
+            requestInterceptor(xhr, options)
+          }
+        },


Does this mean that ajaxSetup will no longer have any effect on requests made through this adapter, or does jQuery execute both implementations of beforeSend?

I need to verify this

So it looks like the options are merged via ajaxExtend. If a 'local'beforeSend is defined, then that hook and only that hook is used. If no 'local' beforeSend exists, but one is defined globally via ajaxSetup, then that hook is used. In other words, practically speaking, ajaxSetup will no longer have any effect on requests made through this adapter in regards to beforeSend

So your intent then is to roll this out as a new major version and have a corresponding groveco PR to reimplement the headers using requestInterceptor?

racoleman · 2020-08-11T21:32:47Z

src/http-adapter/index.js

+    }
+
+    return `${path}/`
+  }


Cleanup for a future PR:

if (!type) throw `resource type is ${type}`; let path = `${this.urlPrefix || ''}/${type}/; if (id !== null && id !== undefined) { path += `${id}/`; } return path;

I would feel confident implementing those changes here, but considering the scope I am not sure it is a good idea 😅

AtofStryker · 2020-08-13T16:44:05Z

@racoleman I went through and addressed many of the comments. I also put a bit more diligence into the documentation. Let me know what you think! Also, for the babel stuff, we might want to hold off on that to avoid additional complexity, even though I think it would be an awesome idea to eventually implement those features

AtofStryker · 2020-08-27T18:30:40Z

@jessewagstaff @electrachong mind taking a look at this?

jessewagstaff

This looks like a good first step to providing more http-adapter options without needing jQuery.ajax dependency.

jessewagstaff · 2020-08-31T19:11:44Z

tests/store.spec.js

@@ -522,4 +522,25 @@ describe('Store', function () {
    it.todo('fetches a single resource')
    it.todo('updates the resource with the response')
  })
+
+  describe('request', function () {


Seems manageable to me. These tests seem good to include.

jessewagstaff · 2020-08-31T19:17:58Z

src/store.js

+  request (url, options = {}) {
+    return this._adapter.request(url, options)
+  }


Where do you see this used instead of Backbone's CRUD? Providing direct access to make http requests feels like it goes against the ideals of Backbone.Store but should help with transitioning. If something needed to make a direct request with the http-adapater could it just create a new instance of the http-adapter for itself?

@jessewagstaff there are a few use cases where it is convenient over reinvoking a new instance of the http-adapter, as well as giving more fine grain options for a request. Within one of our grove apps, we have about 8 one-off requests, some which are in XMLHttpRequest, Fetch, or jQueryAjax. It really doesn't fit super well with the ideals of the backbone.store, but allows for a method via proxy to the http-adapter instance for convenience and ease of use. Ideally, this should help with the transition. Ultimately, once those requests are updated to use the same thing that is tested, we can repeal and replace this with something else. Baby steps 🙂 .

electrachong

Reviewing just the first three commits so far. Most comments are just typo / jsdoc suggestions, hope it's not too noisy.

src/http-adapter/index.js

tests/http-adapter.spec.js

electrachong

Looks good overall, just some questions to resolve about what API we want with internal requests and the request interceptors.

src/http-adapter/index.js

src/http-adapter/jquery/index.js

tests/http-adapter.spec.js

electrachong · 2020-09-02T19:53:37Z

src/http-adapter/jquery/index.js

+        type: method,
+        headers,
+        beforeSend (xhr, options) {
+          if (isInternal) {


What would be the implications of just defaulting to always use the interceptors, or alternatively checking if they have been set to something other than the noop default callback and always using them if they were set? Put another way, what is the use case for toggling them off (via isInternal or a hypothetical useInterceptors) if they have already been set?

Bill talked to me about this via Slack,

Potentially if the requests are going to a third party, which many of them do. Or, we just dont use the request method and use something else. we dont want to be sending header data to those API as it is a security issue. We could also just use the interceptors and check the domain on the interceptor and if its not our url prefix / host, bypass it

I’m fine with either way (using useInterceptors flag or making the app responsible for checking whether to apply the logic within the interceptors).

We could also think about whether to permit request-level interceptors and toggle adapter-level interceptors off by default for one-off requests, but I may be overengineering. I'm good with the simplest improvement and making more changes as necessary when use cases reveal themselves.

Bill talked to me about this via Slack,

Potentially if the requests are going to a third party, which many of them do. Or, we just dont use the request method and use something else. we dont want to be sending header data to those API as it is a security issue. We could also just use the interceptors and check the domain on the interceptor and if its not our url prefix / host, bypass it

I’m fine with either way (using useInterceptors flag or making the app responsible for checking whether to apply the logic within the interceptors).

We could also think about whether to permit request-level interceptors and toggle adapter-level interceptors off by default for one-off requests, but I may be overengineering. I'm good with the simplest improvement and making more changes as necessary when use cases reveal themselves.

@electrachong To just keep it simple, I think making the applications responsible for this is the easiest option. This means removing isInternal from the API. Will update shortly 🙂

AtofStryker · 2020-09-08T17:28:55Z

@jessewagstaff @electrachong isInternal is now removed from the API in 936778a.

jessewagstaff

Looking good. Glad the isInternal flag wasn't needed. Left a few minor jsdoc alternatives.

jessewagstaff · 2020-09-08T18:55:12Z

src/http-adapter/index.js

+})
+
+/**
+ * Adapter which works with data over HTTP, based on the options that


Suggested change

* Adapter which works with data over HTTP, based on the options that

* This adapter works with data over HTTP, based on the options that

jessewagstaff · 2020-09-08T18:56:19Z

src/http-adapter/index.js

+ * are passed to its constructor. This class is responsible for
+ * any CRUD operations that need to be carried out with your {@link https://jsonapi.org/ JSON:API} service


Suggested change

* are passed to its constructor. This class is responsible for

* any CRUD operations that need to be carried out with your {@link https://jsonapi.org/ JSON:API} service

* are passed to its constructor. This class is handles any

* CRUD operations that need to be carried out with your {@link https://jsonapi.org/ JSON:API} service

jessewagstaff · 2020-09-08T19:07:03Z

src/http-adapter/index.js

+  }
+
+  /**
+   * Parses a JSON:API resource to a restful url.


Another way to phrase this is as building a url from the type and id resource identifiers. https://jsonapi.org/format/#document-resource-identifier-objects

electrachong

Do you need to bump version prior to merge?

AtofStryker · 2020-09-09T00:56:21Z

Do you need to bump version prior to merge?

@electrachong we do not since we will not be cutting a version immediately after merge. There are a few other PRs we still need to address / merge in. Then, a separate PR will be opened with a version bump, which will be merged in and then tagged for release.

electrachong · 2020-09-09T02:15:05Z

@AtofStryker Maybe I'm misunderstanding, can sync up via slack. These changes aren't backwards compatible because it requires the app to use the interceptors, right? Even if we install backbone.store from npm registry instead of directly from git, I'm thinking it may be better to merge to a release branch than merge breaking changes directly to master without bumping the version. That way when folks reference commit history the package version will accurately reflect the code state. Have limited familiarity with release strategies though so happy to discuss more

AtofStryker · 2020-09-09T14:47:38Z

@electrachong We can sync up 🙂 .

electrachong

Discussed via slack with Bill, good with approving since this seems to be how we've released new versions in the past! As I understand, we primarily install via npm by release tags and aren't too worried about individual commit history reflecting semantic versioning.

AtofStryker added 2 commits August 6, 2020 11:52

move http-adapter to its own folder

ccb4154

[breaking] move http-adapter to jquery directory

d054632

AtofStryker commented Aug 6, 2020

View reviewed changes

tests/http-adapter.spec.js Outdated Show resolved Hide resolved

AtofStryker requested review from electrachong, jessewagstaff and racoleman August 6, 2020 20:58

AtofStryker force-pushed the jquery-adapter-refactor branch from df50f8a to 32fe7c2 Compare August 7, 2020 17:14

AtofStryker commented Aug 7, 2020

View reviewed changes

AtofStryker marked this pull request as ready for review August 7, 2020 17:46

racoleman reviewed Aug 11, 2020

View reviewed changes

AtofStryker force-pushed the jquery-adapter-refactor branch 4 times, most recently from 1da863d to 8c60e55 Compare August 13, 2020 16:41

AtofStryker requested a review from racoleman August 13, 2020 16:43

AtofStryker removed the request for review from racoleman August 27, 2020 19:45

jessewagstaff mentioned this pull request Aug 31, 2020

Remove dist #78

Merged

jessewagstaff previously approved these changes Aug 31, 2020

View reviewed changes

electrachong reviewed Sep 1, 2020

View reviewed changes

AtofStryker added 4 commits September 1, 2020 18:55

implement the HttpAdapter base class with tests

1895b9d

implement the jqueryHttpAdapter by extending from HttpAdapter base class

9e0a485

Add request method to store

d1082d1

add .vscode to the .gitignore

c4ad894

AtofStryker dismissed jessewagstaff’s stale review via c4ad894 September 1, 2020 22:55

AtofStryker force-pushed the jquery-adapter-refactor branch from 8c60e55 to c4ad894 Compare September 1, 2020 22:55

AtofStryker requested review from jessewagstaff and electrachong September 1, 2020 23:05

electrachong reviewed Sep 2, 2020

View reviewed changes

remove isInternal from the http-adapter spec

936778a

AtofStryker requested a review from electrachong September 8, 2020 17:28

jessewagstaff approved these changes Sep 8, 2020

View reviewed changes

electrachong approved these changes Sep 8, 2020

View reviewed changes

electrachong suggested changes Sep 8, 2020

View reviewed changes

AtofStryker requested a review from electrachong September 9, 2020 00:56

electrachong approved these changes Sep 9, 2020

View reviewed changes

ssepasi approved these changes Sep 14, 2020

View reviewed changes

AtofStryker merged commit b0a3750 into master Sep 14, 2020

This was referenced Sep 14, 2020

Fix unit tests #79

Merged

bump to version 0.5.0 for major release #80

Merged

	* Adapter which works with data over HTTP, based on the options that
	* This adapter works with data over HTTP, based on the options that

		* are passed to its constructor. This class is responsible for
		* any CRUD operations that need to be carried out with your {@link https://jsonapi.org/ JSON:API} service

Jquery adapter refactor #76

Jquery adapter refactor #76

Conversation

AtofStryker commented Aug 6, 2020 • edited Loading

Strategy

Rationale

External API

request method added to public API via Store -> HttpAdapter

requestInterceptor callback added

responseInterceptor callback added

Internal API

Moving serializeRequest and buildUrl to base class

Adding isInternal argument to _makeRequest

Default application/vnd.api+json header arguments in _http

Common FAQ

Is the public API the same?

Are Backbone Collections behind the scenes using our default headers

Will the new request method use the interceptors?

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

racoleman 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

AtofStryker commented Aug 13, 2020

AtofStryker commented Aug 27, 2020

jessewagstaff 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

electrachong left a comment

Choose a reason for hiding this comment

electrachong left a comment

Choose a reason for hiding this comment

electrachong Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Sep 8, 2020

jessewagstaff 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

electrachong left a comment

Choose a reason for hiding this comment

AtofStryker commented Sep 9, 2020

electrachong commented Sep 9, 2020

AtofStryker commented Sep 9, 2020

electrachong left a comment

Choose a reason for hiding this comment

AtofStryker commented Aug 6, 2020 •

edited

Loading

`request` method added to public API via `Store` -> `HttpAdapter`

`requestInterceptor` callback added

`responseInterceptor` callback added

Moving `serializeRequest` and `buildUrl` to base class

Adding `isInternal` argument to `_makeRequest`

Default `application/vnd.api+json` header arguments in `_http`

Will the new `request` method use the interceptors?

electrachong Sep 2, 2020 •

edited

Loading