Promise aware headers

[sandstrom]

Sometimes the headers need to be set after resolving some promise. For example this issue or use-cases where an authorization token is stored in async storage such as IndexedDB.

I'd suggest switching the method to buildHeaders (or something similarly named), which can return a hash or a promise that resolves with a hash.

Thoughts?

[alexlafroscia]

That is pretty interesting... Hadn't thought about it much but I could see the value.

If someone needed this feature and wanted to make a PR, I would be happy to review and accept a PR to that effect.

[joeyespo]

Just ran into this myself.

I tried using async:

headers: computed(async function() {    // (proof-of-concept)
  const headers = {};
  const token = await store.restore();  // (pseudocode)
  if (token) {
    headers.Authentication = `Bearer ${token}`;
  }
  return headers;
})

It appeared to work, but basically had no effect at runtime. Likely due to it not being promise-aware.

I ended up using session.data.authenticated.token after seeing Alex's response. That worked for now, however I can see this being a problem again on another project that don't use ember-simple-auth.

So 👍 (both to @alexlafroscia for unknowingly fixing my problem 😄, and to this issue)

[alexlafroscia]

Hey @joeyespo, nice to see you!

Just to play devil's advocate a bit, and to explore a solution that doesn't involve making the Header creation async, would an ember-concurrency task work for you? I generally think that's a better pattern for handling async code in Ember that is outside a route. It might look something like this (assuming you have an async action that retrieves some kind of token for the user to sign requests with):

// app/services/session.js
import Service from '@ember/service';
import { task } from 'ember-concurrency';

export default Service.extend({
  userToken: undefined,

  authenticateUser: task(function* () {
    const token = yield doSomeAsyncActionToGetTheToken();
 
    this.set('userToken', token);
  })
});

// app/service/ajax.js
import AJAXService from 'ember-ajax/services/ajax';
import { inject as service } from '@ember/service';

export default AJAXService.extend({
  session: service(),
  headers: computed('session.userToken', function() {
    return {
      token: this.get('session.userToken')
    };
  })
});

This way, you explicitly handle the asynchrony by being able to look at session.authenticateUser.isRunning to determine what to do while the promise is in flight. If you don't want to do that, you can just look at session.userToken directly and get whatever the most recent value is.

This ends up being better, both here and in other areas of Ember, since Ember's KVO doesn't really handle promises that well (and in the cases where it does through an PromiseProxy or something, it can end up with some troll-y behavior to people trying to reason about the codebase). As I mentioned above, the Ember Concurrency task allows you to bind explicitly to the value that the task sets, and to observe whether the task is currently in-flight to handle those cases as well if necessary.

[alexlafroscia]

You could bind to session.authenticateUser.lastSuccessful.value as well, if the task was defined with

function*() {
  return doSomeAsyncActionToGetTheToken();
}

That's a bit cleaner; it'll be null until the task successfully completes. Or, if your linter complains about a missing yield like mine does, this also works:

function*() {
  return yield doSomeAsyncActionToGetTheToken();
}

(although I find using both return and yield to be a bit confusing). An extra line of code makes it much easier to read and achieves the same effect:

function*() {
  const token = yield doSomeAsyncActionToGetTheToken();
  return token;
}

Note, if you try it out: something has to explicitly call .perform() on authenticateUser, like an initializer. It's not like a computed property that is automatically evaluated when referenced (this confused me for a bit before I remembered how Ember Concurrency works).

[sandstrom]

Yes, you could solve this outside ember-ajax. There are parts of Ember Data that are promise-aware too, although it could also be sync with async handling outside the framework.

My view is that if some parts of an API may be used in both sync/async, it's nice if it can handle both.

I think it would be great if we could support an async buildHeaders method and I'm willing to promise a PR. As long as you are not against the idea (in that case I'll do it in a fork instead).

[alexlafroscia]

I'm actually thinking about coming up with a pattern that we could extend to other properties like host and namespace.

What do you think about creating a pattern where each property could be defined by either a property or method following the pattern build__NAME_OF_PROPERTY__. So for example, there might be an internal async _buildHeaders method that looks for a public buildHeaders method and calls/resolves it. In the absence of the buildHeaders method it would get the headers property, which could be computed or not, up to the user.

We could then extend this pattern to buildNamespace, buildHost, etc and come up with a consistent pattern (and reusable code) around setting any property either sync or async.

[alexlafroscia]

To be clear, I'm definitely not against the idea of being able to set them asynchronously. I just don't want to open the door to the headers computed property returning a promise, because I think that's a recipe for code that behaves in unexpected ways.

I'm going to get on a plane pretty soon, so I'll put a demo together of my idea around being able set properties sync or async and make a PR that we can look at together.

[sandstrom]

If I understand you correctly, ember-ajax would look for buildY (returning a promise) and then fallback to y (returning a value) if the build-prefixed method doesn't exist. Sounds good!

Enjoy! ✈️

[alexlafroscia]

Yup! Exactly. And buildY would be called with the same arguments passed to the actual get/post/whatever method, in case that's important.

I didn't get a chance to experiment with this on the plane, but will get something together by next week.

[alexlafroscia]

@sandstrom As a user of ember-ajar (which I am actually not) how do you feel about including ember-maybe-import-regenerator as a dependency? The implementation will be quite a bit cleaner and easier to maintain if I port some of this new possibly-async logic to async/await. The dependency might add as much as 2kb (nothing in the scheme of an Ember app, IMO) but if the user is already including the babel polyfill or ember-concurrency it'll add nothing. Thoughts?

[alexlafroscia]

Hmm... I want to rewrite with async/await but our custom Promise class (that preserves a reference to the xhr of the request, for cancellation) won't work with it...

I'm thinking that this will actually require a 4.0.0 release, which will change some of the semantics for accessing the xhr. I like the idea of doing something like

import { getXHR } from 'ember-ajax';

const promise = this.get('ajax').request('/foo');
const xhr = getXHR(promise);

// or

const promise = this.get('ajax').request('/foo').then(response => response);
const xhr = getXHR(promise);

something where you pass the promise (or child promise) in and get an XHR back, rather than accessing a property that we need to propagate through the custom promises.

[sandstrom]

I don't mind ember-maybe-import-generator!

We should try to avoid an extra call to dig out the xhr. Would it be possible to move from RSVP to native promises? That would solve the await issue without introducing a getXHR method.

Also, have you considered dropping jQuery from ember-ajax (ember.js itself is slowly moving away from jQuery) and use native XHR or the fetch-framework?

It would be a good idea to make both changes in one swoop, since removing jQuery will touch a lof the promise internals anyway + it's a breaking change (although minor, but afaik it will affect the options hash).

[alexlafroscia]

We should try to avoid an extra call to dig out the xhr. Would it be possible to move from RSVP to native promises? That would solve the await issue without introducing a getXHR method.

I think the problem isn't RSVP vs Promise, but rather that we have to make a sub-class that overrides .then to preserve a reference to the XHR. I'll try extending from the native Promise instead of RSVP.Promise and see if the issue persists.

Also, have you considered dropping jQuery from ember-ajax (ember.js itself is slowly moving away from jQuery) and use native XHR or the fetch-framework?

The whole idea is to be a wrapper for $.ajax so I don't think we'll remove the dependency. I could eventually see us importing a custom build of jQuery that only includes the .ajax method eventually, once Ember actually doesn't ship with jQuery.

To re-write to use XHR directly would mean re-implementing $.ajax ourselves, which I'm not interested in. If people want to avoid $.ajax then they should, IMO, just use ember-fetch. I don't even use this addon anymore and would actively recommend people use fetch instead. I just maintain the repo for all you lovely folks 😁

[sandstrom]

Ah, you may be right! ( was just guessing it was because of the RSVP)

I see, I always thought the plan was to move this addon off jQuery. But in that case I'll see if we can switch over to native/polyfilled fetch or ember-fetch.

That way this addon won't need to see much churn or breaking changes, which is probably ideal for people wanting to use jQuery.ajax. And others can use ember-fetch or something else for fetch-based XHR-code.

[alexlafroscia]

I see, I always thought the plan was to move this addon off jQuery

Nope, I don't think so. Out of curiosity, what would that look like?

I personally believe that fetch is the future. Unfortunately, it does not support all of the features necessary to be a 100% replacement for AJAX, but by and large I think it fits most people's needs and is a better API. If fetch supported progress events I would probably just stop maintaining this altogether, but I know some people have a strong preference for AJAX over Fetch or require some XHR features that Fetch doesn't have a story for yet.

[alexlafroscia]

@sandstrom check out that commit when you get a chance ^^

The tests are going to fail, due to the async/await issue I mentioned above. Debugging the tests, I found that in a code example like this:

const promise = service.request('/foo');

console.log(promise.xhr);

the code that would normally attach the xhr object actually runs after the console.log would happen. I don't know if it's just a race condition or more than that -- the promise in the above example is actually an instance of Promise, not the custom AJAXPromise that we have to use.

Aside from the .xhr issue, the rest of the code works.

[alexlafroscia]

Thinking about it more, I suppose it makes sense. An async method has to return a regular, vanilla Promise because it has no idea what the method would eventually return.

[alexlafroscia]

Unfortunately, this whole .xhr property thing is causing quite the headache here. I started thinking about a solution with an optional token, similar to how Promise cancelation will work in the future. The problem is that all async properties (like headers would need to be resolved before we create the xhr object. The question arises, whether the solution around the xhr is a property or some token thing to fetch the object -- what should the value be while the properties are being asynchronously loaded? Would promise.xhr just be undefined, null... something else?

I think that the best option is honestly to just asynchronously create the headers and then synchronously pass them on a per-request basis. You could probably add a method to the service that does it for every request if that's what you need.

If forking was the solution you were going to go for, how were you going to solve the problem of the value of promise.xhr while the headers are resolving? I'm out of ideas, but happy to continue working on this if we can solve that problem.

[sandstrom]

Yes, it was something like that (your commit) that I had in mind.

But we're just going to switch to ember-fetch instead, since we are moving away from jQuery.

Since this addon is ment to keep using jQuery Ajax I don't think it makes sense to spend too much time on it. There are other solutions to async headers, that doesn't require any modifications to this addon (as you mentioned).

So I think we should just close this issue.

[alexlafroscia]

Alright, sounds good to me. It's a shame I couldn't get this working, I love the idea of having any property be settable in an async manner but that leaking .xhr just makes it impossible.