Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ember-fetch + fastboot + ember-data sadness #38

Open
stefanpenner opened this issue Jun 15, 2017 · 16 comments
Open

ember-fetch + fastboot + ember-data sadness #38

stefanpenner opened this issue Jun 15, 2017 · 16 comments

Comments

@stefanpenner
Copy link
Collaborator

stefanpenner commented Jun 15, 2017

fastboot and fetch share the same extension point in ember-data, and today they do not work together.

Long-term, I suspect fastboot should provide a XMLHTTPRequest global, and a fetch global. That way it doesn't interfere in high level code, and would also "just work" with things like pretender/mirage.

@0xadada
Copy link

0xadada commented Feb 18, 2018

I'm running into this exact problem now. My app uses fastboot, ember-data, ember-fetch, and I'm in the process of removing jquery and everything is working, except fastboot fetches with the following error:

Error while processing route: index Cannot read property 'catch' of undefined TypeError: Cannot read property 'catch' of undefined
    at Class.ajax (/Users/ron/mirai-audio/mir/tmp/broccoli_merge_trees-output_path-IEMm2qLU.tmp/assets/addon-tree-output/ember-fetch/mixins/adapter-fetch.js:200:1)
    at Class.findAll (/Users/ron/mirai-audio/mir/tmp/broccoli_merge_trees-output_path-IEMm2qLU.tmp/assets/addon-tree-output/modules/ember-data/adapters/rest.js:473:1)
    at _findAll (/Users/ron/mirai-audio/mir/tmp/broccoli_merge_trees-output_path-IEMm2qLU.tmp/assets/addon-tree-output/modules/ember-data/-private.js:8501:1)
    at Class._fetchAll (/Users/ron/mirai-audio/mir/tmp/broccoli_merge_trees-output_path-IEMm2qLU.tmp/assets/addon-tree-output/modules/ember-data/-private.js:10967:1)
    at Class.findAll (/Users/ron/mirai-audio/mir/tmp/broccoli_merge_trees-output_path-IEMm2qLU.tmp/assets/addon-tree-output/modules/ember-data/-private.js:10938:1)
    at Class.model (/Users/ron/mirai-audio/mir/tmp/broccoli_merge_trees-output_path-IEMm2qLU.tmp/assets/mir/routes/index.js:23:1)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

this line: 192

ajax(url, type, options) {
const requestData = {
url,
method: type,
};
const hash = this.ajaxOptions(url, type, options);
return this._ajaxRequest(hash)
.catch((error, response, requestData) => {
throw this.ajaxError(this, response, null, requestData, error);
})

@nlfurniss
Copy link
Collaborator

@0xadada you can get around this issue with these instructions.

@0xadada
Copy link

0xadada commented Mar 2, 2018

@nlfurniss unfortunately not because I've also removed jQuery in my app as a dependency, and the then().catch() semantics of ember fetch don't yet align with fastboot expectations.

@nlfurniss
Copy link
Collaborator

Did you try adding the initializer to override fastboot per the README?

@0xadada 0xadada mentioned this issue Mar 7, 2018
8 tasks
@Duder-onomy
Copy link

Duder-onomy commented Mar 8, 2018

I also ran into an nearly identical issue with ember-data ember-fetch ember-cli-fastboot and ember-simple-auth

Ember-fetch's ajaxOption's does not call super, which results in any other extensions of your application adapter not having the opportunity to set headers or anything like that. The current implementation of ember-fetch's ajaxOptions do not allow this to run.

This breaks ember-simple-auth because the ember-simple-auth's adapter mixin needs to set your auth header.

My workaround is to manually set the headers I need after ember-fetch has run:

  ajaxOptions(...args) {
    const options = this._super(...args);

    get(this, 'session').authorize(get(this, 'authorizer'), (headerName, headerValue) => {
      options.headers[headerName] = headerValue;
    });

    return options;
  },

@0xadada
Copy link

0xadada commented Mar 8, 2018

@nlfurniss that worked! I hadn't taken that step, because it wasn't in the README back when i first switched to ember-fetch. Thanks!

@tchak
Copy link
Collaborator

tchak commented Apr 7, 2018

I just want to mention that we are moving fetch support to Ember Data itself : emberjs/data#5386

@ehubbell
Copy link

@Duder-onomy I'm running into the same issue (using the same libraries) as you explain above. Curious, would you mind elaborating on where and how you manually set those headers?

@Duder-onomy
Copy link

Duder-onomy commented Jun 28, 2018

@ehubbell Totally.

The latest ember-simple-auth has deprecated authorizers... So I had to change some things a few weeks ago. Now that ajaxOptions hook I pasted above looks a little different.

Because we are using basic auth to protect our staging environment, and we are using fastboot, and simple auth uses the 'Authorization' header, and fastboot will not let us change that default... we had to change the name of our access token key to something that does not use Authorization. We used X-Access-Token and had to subsequently change our authorization code in devise to use a diff key.

Here is my entire application adapter:

import DS from 'ember-data';
import DataAdapterMixin from 'ember-simple-auth/mixins/data-adapter-mixin';
import AdapterFetch from 'ember-fetch/mixins/adapter-fetch';
import { get } from '@ember/object';
import { inject as service } from '@ember/service';
import config from '../config/environment';

const { JSONAPIAdapter } = DS;

export default JSONAPIAdapter.extend(DataAdapterMixin, AdapterFetch, {
  session: service(),

  authorizer: 'authorizer:application',

  coalesceFindRequests: true,

  host: config.APP.apiBase,
  namespace: config.APP.apiNamespace,

  headers: {
    'X-Requested-with': 'XMLHttpRequest',
  },

  ajaxOptions(...args) {
    const options = this._super(...args);
    const accessToken = get(this, 'session.data.authenticated.access_token');

    options.headers['X-Access-Token'] = accessToken;

    options.headers['Content-Type'] = 'application/vnd.api+json';

    return options;
  },
});

Hope that helps.

@ehubbell
Copy link

ehubbell commented Jul 5, 2018

@Duder-onomy Thanks for the help! Turns out I had my fastboot directory inside my app directory hence the issues my ajax service. Once I fixed that and installed something similar to your code above, everything's working.

@nlfurniss
Copy link
Collaborator

@mdbiscan yes, here is an example

@mdbiscan
Copy link

mdbiscan commented Aug 22, 2018

@nlfurniss sorry, hit delete by accident. Thanks for responding. Looks like it's the right track. I have another error to get past, but its related to a DRF adapter mixin.

@mdbiscan
Copy link

mdbiscan commented Aug 23, 2018

One of my issues is needing to pass HTTPOnly cookies. In my adapter headers property, I had to add this:

if(fastboot.isFastBoot) {
  headers['Cookie'] = fastboot.get('request.headers.cookie');
}

@0xadada
Copy link

0xadada commented May 6, 2019

Solutions!

@stefanpenner
Here is how I've finally solved the problem, i'm also using ember-data with ember-fetch running in an ember-cli-fastboot with ember-simple-auth and have removed jQuery as a dependency.

Here is my entire application adapter:

re: ☝️ @Duder-onomy your application adapter uses authorizers, which has since become a deprecated pattern in ember-simple-auth.

Authorizers and the session service's authorize method are deprecated and will be removed from Ember Simple Auth 2.0. The concept seemed like a good idea in the early days of Ember Simple Auth, but proved to provide limited value for the added complexity. To replace authorizers in an application, simply get the session data from the session service and inject it where needed.

When used with ember-fetch the ajaOptions doesn't work well, and the authorize method will not be called. The headers computed property must be used instead. Here is my application adapter using ember-fetch:

// app/adapters/application.js
import JSONAPIAdapter from 'ember-data/adapters/json-api';
import AdapterFetchMixin from 'ember-fetch/mixins/adapter-fetch';
import config from 'mir/config/environment';
import { computed } from '@ember/object';
import { inject as service } from '@ember/service';

export default JSONAPIAdapter.extend(AdapterFetchMixin, {
  session: service(),

  host: config.DS.host,
  namespace: config.DS.namespace,

  headers: computed('session.data.authenticated.token', function() {
    const headers = {};
    headers['Content-Type'] = 'application/vnd.api+json';
    if (this.session.isAuthenticated) {
      const token = this.session.data.authenticated['access_token'];
      headers['Authorization'] = `Bearer ${token}`;
    }
    return headers;
  })
});

I also followed the ember-fetch README, to disable the ajax instance initializer on fastboot:

// fastboot/initializers/ajax.js
export default {
  name: 'ajax-service',
  initialize() {
    // noop
    // This is to override Fastboot's initializer which prevents ember-fetch from working
    // https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/fastboot/initializers/ajax.js
  }
}

One of the nice benefits of this approach is that it helps paves the way to removing jQuery, especially with $.ajax no longer used by ember-data.

@snewcomer
Copy link

Note on setting ajaxOptions. I would follow what was recommended in the below . Also, headers as a computed may be another source of a memory leak. Changing to a static getter e.g. get header may have fixed leaking an internal part of Ember.

Just sending out a message in a bottle in case anybody picks it up. Your app may be slightly different, so make sure you have a memory leak before changing from a computed to getter.

Ref - emberjs/data#5061 (comment)

@snewcomer
Copy link

@allthesignals ref issue for container memory leaks

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

No branches or pull requests

8 participants