Ember Data Adapter Mixin to use fetch #31

nlfurniss · 2017-05-25T05:16:34Z

I would have liked to do some more testing of the adapter methods, let's discuss how best to do that.

kratiahuja · 2017-05-25T05:57:03Z

@nlfurniss how will this work with things like bigpipe?

nlfurniss · 2017-05-25T06:08:25Z

Yup it works fine with bigpipe (used v-web to test this). The only thing I need to confirm is that the proper tracking headers are attached to requests BPR sends. Have sent Mark and Rajul an email to figure out how to confirm.

stefanpenner · 2017-05-25T14:20:48Z

I like this as a stepping stone for ember-data users to use ember-fetch 👍

stefanpenner · 2017-05-25T14:21:21Z

README.md

@@ -28,11 +28,14 @@ export default Ember.Route.extend({
 });
 ```

+### Use with Ember Data
+To have Ember Data utilize `fetch` instead of jQuery.ajax to make calls to your backend, extend your project's `application` adapter with the `adapter-fetch` mixin.


lets give a quick code example.

stefanpenner · 2017-05-25T14:21:38Z

README.md

 further docs: https://github.com/github/fetch

 ### Browser Support

-* evergreen / IE9+ / Safari 6.1+ https://github.com/github/fetch#browser-support
+* evergreen / IE10+ / Safari 6.1+ https://github.com/github/fetch#browser-support


ah is the polyfil IE10+ only now?

Yup, according to their readme: https://github.com/github/fetch#browser-support

stefanpenner · 2017-05-25T14:22:40Z

addon/mixins/adapter-fetch.js

+ * @param {Object} options
+ */
+export function mungOptionsForFetch(options) {
+  const _options = assign({


tiny nit pick, but can we make the param be _options but the one used throughout the function option I think this reads nicer.

stefanpenner · 2017-05-25T14:23:53Z

addon/mixins/adapter-fetch.js

+        throw this.ajaxError(null, response, requestData);
+      })
+      .catch((error, response, requestData) => {
+        throw this.ajaxError(error, response, requestData);


As implemented this catch will also catch all errors thrown in the then which precedes it. This includes its own this.ajaxError That seems funky.

I believe simply re-ordering the then and the catch would address this issue.

this._ajaxRequest(hash) .catch((error, response, requestData) => { throw.this.ajaxError(error, response, requestData); }).then(response => { if (response.ok) { const bodyPromise = response.json(); return this.ajaxSuccess(response, bodyPromise, requestData); } throw this.ajaxError(null, response, requestData); });

stefanpenner · 2017-05-25T14:27:01Z

addon/mixins/adapter-fetch.js

+          body,
+          requestData
+        );
+      } catch (error) {


this try/catch is redundant and can be removed. the caught error will already be caught be the then handler it is being called in.

stefanpenner · 2017-05-25T14:28:41Z

addon/mixins/adapter-fetch.js

+          requestData
+        );
+      } catch (e) {
+        returnedError = e;


any downside to letting this just throw ?

nlfurniss · 2017-05-25T22:35:00Z

Updated with @stefanpenner's suggestions

nlfurniss · 2017-05-26T08:47:28Z

so phantom doesn’t have Map or Headers, so my test of the headersToObject helper is failing.

do you want to shim Map/Headers, leave out the test, other?

tchak · 2017-05-31T10:02:10Z

@nlfurniss polyfilled Headers should be available as import { Headers } from 'ember-fetch';

cibernox · 2017-05-31T13:46:55Z

@nlfurniss This is fantastic 👏

Counting the seconds to use it.

cibernox · 2017-05-31T16:23:39Z

addon/mixins/adapter-fetch.js

+  // GET and HEAD requests can't have a `body`
+  if (options.data && Object.keys(options.data).length) {
+    if (options.method === 'GET' || options.method === 'HEAD') {
+      options.url += `?${serialiazeQueryParams(_options.data)}`;


I found one error. This does not serialize nested objects properly.

Example:

serialiazeQueryParams(_options.data) with this implementation generates page=%5Bobject%20Object%5D.

The expected queryParams that is generated by $.ajax is page%5Bnumber%5D=1&page%5Bsize%5D=3

I've been playing in the console, this snippet seems to work well for arbitrary nested objects:

function serialiazeQueryParams(queryParamsObject, prefix = null) { return Object.keys(queryParamsObject).sort().map((key) => { let value = queryParamsObject[key]; if (prefix) { key = `${prefix}[${key}]`; } if (value !== null && typeof value === 'object') { return serialiazeQueryParams(value, key); } else { return `${encodeURIComponent(key)}=${encodeURIComponent(value)}`; } }).join('&'); }

I haven't checked how arrays should work with arrays.

For what is worth, this is the implementation of $.param, which is the thing used by $.ajax: https://github.com/jquery/jquery/blob/master/src/serialize.js#L17-L90

We spoke offline and this functionality has been added, along with tests

cibernox · 2017-05-31T16:48:27Z

addon/mixins/adapter-fetch.js

+  // GET and HEAD requests can't have a `body`
+  if (options.data && Object.keys(options.data).length) {
+    if (options.method === 'GET' || options.method === 'HEAD') {
+      options.url += `?${serialiazeQueryParams(_options.data)}`;


I've been playing in the console, this snippet seems to work well for arbitrary nested objects:

function serialiazeQueryParams(queryParamsObject, prefix = null) { return Object.keys(queryParamsObject).sort().map((key) => { let value = queryParamsObject[key]; if (prefix) { key = `${prefix}[${key}]`; } if (value !== null && typeof value === 'object') { return serialiazeQueryParams(value, key); } else { return `${encodeURIComponent(key)}=${encodeURIComponent(value)}`; } }).join('&'); }

I haven't checked how arrays should work with arrays.

For what is worth, this is the implementation of $.param, which is the thing used by $.ajax: https://github.com/jquery/jquery/blob/master/src/serialize.js#L17-L90

…uery.ajax

cibernox · 2017-06-01T19:26:31Z

@nlfurniss Why was this closed?

nlfurniss · 2017-06-01T19:39:08Z

an accident of those history fixes I did :-\

But it's back!

cibernox · 2017-06-01T19:39:48Z

@nlfurniss You scared the s**** out of me ☺️

cibernox · 2017-06-02T19:05:40Z

addon/mixins/adapter-fetch.js

+   * @override
+   */
+  _ajaxRequest(options) {
+    const _options = mungOptionsForFetch(options);


I found another issue. This is not respecting the Headers, as in jQuery the options are set using a beforeSend function and in fetch headers must be a Headers object passed on the options.

Added a PR for this.

@stefanpenner, how do you feel about adding ED to this addon?

Pull request

cibernox · 2017-06-02T21:37:11Z

addon/mixins/adapter-fetch.js

+      })
+      .then((response) => {
+        if (response.ok) {
+          const bodyPromise = response.json();


This line also fails if the server responds with a 204 No content, since JSON.parse throws an error trying to parse an empty response.

Is using the Content-Length header reliable enough? or is 204 an edge case?

let bodyPromise; if (response.ok && response.status !== 204) { bodyPromise = response.json(); } else { bodyPromise = {}; } ...

No idea. I've asked in slack what the right way of handing 204s is.

cibernox · 2017-06-07T10:33:50Z

After some debugging I found that this approach doesn't work if you also run your app in fastboot. I opened this issue in there: ember-fastboot/ember-cli-fastboot#419 (comment)

The tl;dr is that fastboot has an initializer that also replaces the _ajaxRequest method on all adapters to use node-ajax. This mixin and that initializer are essentially fighting.

The solution I see is to put this mixing in a standalone addon, not in here, so that addon can explicitly initialize itself after ember-cli-fastboot and trump over it.

Another approach if the people in ember-cli-fastboot agree would be to move the ember-data compatibility layer elsewhere. Perhaps to ember-data itself, perhaps to an ember-data-fastboot-adapter people has to install.

stefanpenner · 2017-06-12T17:49:36Z

The tl;dr is that fastboot has an initializer that also replaces the _ajaxRequest method on all adapters to use node-ajax. This mixin and that initializer are essentially fighting.

We will need to tackle this independent of this issue. I believe fastboot XHR/FETCH interactions needs some love.

stefanpenner · 2017-06-12T17:50:07Z

addon/mixins/adapter-fetch.js

+    var i, len, key;
+
+    if (prefix) {
+      if (isArray(obj)) {


Array.isArray

stefanpenner · 2017-06-12T17:50:13Z

addon/mixins/adapter-fetch.js

+ */
+export function serialiazeQueryParams(queryParamsObject) {
+  var s = [], rbracket = /\[\]$/;
+  function isArray(obj) {


we don't need this helper, Array.isArray is available in our target browsers and is also faster.

stefanpenner · 2017-06-12T17:50:55Z

addon/mixins/adapter-fetch.js

+  function isArray(obj) {
+    return Object.prototype.toString.call(obj) === '[object Array]';
+  }
+  function add(k, v) {


lets not redefine this method on each invocation of serialiazeQueryParams, instead hoist it up.

stefanpenner · 2017-06-12T17:51:21Z

addon/mixins/adapter-fetch.js

+ * @returns {String}
+ */
+export function serialiazeQueryParams(queryParamsObject) {
+  var s = [], rbracket = /\[\]$/;


rbracket should likely be const RRACKET = /... at the top of the module (no need to redefine it.

stefanpenner · 2017-06-12T17:54:05Z

@cibernox you mentioned the fastboot thing, do you feel that is a blocker here?

cibernox · 2017-06-12T19:51:58Z

@stefanpenner I'd say that the inability if use ember-fetch (this mixin only, the rest works well) along with fastboot might be a problem for ppl building PWAs without jquery.

TBH, I think that in this case fastboot's approach is a bit intrusive.

kratiahuja · 2017-06-12T19:58:00Z

TBH, I think that in this case fastboot's approach is a bit intrusive.

Why is fastboot's approach a bit intrusive? I think the problem is the union with ember-data. If you aren't using ember-data this would work out of the box (even in fastboot). If you chose to use ember-data, then ofcourse you will need to provide a way to override the behavior in fastboot like you are doing here with the mixin.

Fix headers of Ember-Data Mixin

stefanpenner · 2017-06-14T16:28:11Z

Why is fastboot's approach a bit intrusive?

I think fastboot may want to not monkey patch ember-data, but rather provide an XMLHTTPRequest and Fetch global. I think that would be less intrusive, and work with more libraries (pretender/mirage etc). This idea needs more investigation/exploration etc...

chatted offline

stefanpenner · 2017-06-15T17:01:07Z

released as v3.0.0 🎉

issue for fastboot + ED compat: #38

kratiahuja · 2017-06-15T18:51:40Z

I think fastboot may want to not monkey patch ember-data, but rather provide an XMLHTTPRequest and Fetch global. I think that would be less intrusive, and work with more libraries (pretender/mirage etc). This idea needs more investigation/exploration etc...

Agreed the monkey patching is bad. but I also think ember-data should be providing an network interface for fastboot or anyone else to define. This way even ember-fetch won't need to define a mixin but just defines the interface for ember data.

stefanpenner requested changes May 25, 2017

View reviewed changes

nlfurniss force-pushed the master branch from 46bfb2c to f5c671c Compare May 25, 2017 22:34

nlfurniss force-pushed the master branch 3 times, most recently from 6c87c87 to 9f15853 Compare May 30, 2017 08:14

cibernox requested changes May 31, 2017

View reviewed changes

nlfurniss force-pushed the master branch from 9f15853 to 1cac20f Compare May 31, 2017 18:17

cibernox approved these changes May 31, 2017

View reviewed changes

nlfurniss force-pushed the master branch from 67d5f57 to 34cfba9 Compare May 31, 2017 23:02

nlfurniss closed this Jun 1, 2017

nlfurniss force-pushed the master branch from 34cfba9 to c178bf8 Compare June 1, 2017 19:24

Add adapter mixin which enables Ember Data to use fetch instead of jQ…

d4776bc

…uery.ajax

nlfurniss reopened this Jun 1, 2017

Respect adapter headers if present

5f48c57

cibernox requested changes Jun 2, 2017

View reviewed changes

cibernox previously requested changes Jun 2, 2017

View reviewed changes

cibernox mentioned this pull request Jun 6, 2017

Ajax initializer prevents users to override _ajaxRequest ember-fastboot/ember-cli-fastboot#419

Closed

stefanpenner requested changes Jun 12, 2017

View reviewed changes

Nathaniel Furniss and others added 3 commits June 12, 2017 16:32

Cleanup jquery query param serialization

d0e600b

Merge pull request #2 from cibernox/fix-headers-of-ED-mixin

c5396e9

Fix headers of Ember-Data Mixin

Handle empty response

23ba2ae

stefanpenner approved these changes Jun 14, 2017

View reviewed changes

Support Ember.merge

cf354ba

stefanpenner merged commit 09e10ef into ember-cli:master Jun 15, 2017

Ember Data Adapter Mixin to use fetch #31

Ember Data Adapter Mixin to use fetch #31

Conversation

nlfurniss commented May 25, 2017

kratiahuja commented May 25, 2017

nlfurniss commented May 25, 2017

stefanpenner commented May 25, 2017

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

nlfurniss commented May 25, 2017

nlfurniss commented May 26, 2017

tchak commented May 31, 2017

cibernox commented May 31, 2017

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

cibernox commented Jun 1, 2017

nlfurniss commented Jun 1, 2017

cibernox commented Jun 1, 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

cibernox commented Jun 7, 2017

stefanpenner commented Jun 12, 2017

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

stefanpenner commented Jun 12, 2017

cibernox commented Jun 12, 2017

kratiahuja commented Jun 12, 2017

stefanpenner commented Jun 14, 2017 • edited Loading

stefanpenner commented Jun 15, 2017

kratiahuja commented Jun 15, 2017

cibernox commented Jun 1, 2017 •

edited

Loading

stefanpenner commented Jun 14, 2017 •

edited

Loading