Skip to content
This repository has been archived by the owner on Jan 20, 2019. It is now read-only.

[Proposal] ic-ajax -> ember-fetch #19

Closed
1 task
stefanpenner opened this issue Jul 1, 2015 · 24 comments
Closed
1 task

[Proposal] ic-ajax -> ember-fetch #19

stefanpenner opened this issue Jul 1, 2015 · 24 comments

Comments

@stefanpenner
Copy link
Contributor

Lets begin this discussion.

What is ember-fetch?

Why is this discussion happening now and not earlier?

  • ember 2 no longer supports IE8, and github/fetch is IE9+
  • fetch is a spec primitive of the DOM, it stands to reason we should prefer that over $.ajax
  • maybe we can catch issues to help adapt/improve/add to the spec
  • $.ajax is crazy quirky
  • uploads have progress in the request object (unsure if the polifill does this or not actually, need to verify)

Cons:

  • its not $.ajax which, although not a spec thing, is basically a primitive many devs are used to
  • features like $.ajaxPrefilter are not obviously a thing in fetch (maybe some other approach ?)
  • .... ?

Noop:

  • still no formal cancelation

things to check:

  • polyfill support upload progress?
@jamesarosen
Copy link

Con: fetch resolves successfully on 4xx and 5xx errors, but $.ajax rejects.

@bcardarella
Copy link
Contributor

@jamesarosen that seems like a huge con

@cibernox
Copy link
Contributor

cibernox commented Jul 1, 2015

Con: in those browsers where the real fetch is available, pretender won't intercept requests properly

@jamesarosen
Copy link

@cibernox similarly for Sinon and other XMLHttpRequest fakers

@stefanpenner
Copy link
Contributor Author

Con: fetch resolves successfully on 4xx and 5xx errors, but $.ajax rejects.

this is both a CON and a PRO.

Think other HTTP libs, typically they don't throw an exception if the response was 500, rather they give you an object with the status code. Rather they only fail if the connection itself failed, or if some code exploded.

@stefanpenner
Copy link
Contributor Author

Con: in those browsers where the real fetch is available, pretender won't intercept requests properly

incorrect, ember-fetch never falls back to native. (although this may not be obvious)

@Serabe
Copy link

Serabe commented Jul 1, 2015

@stefanpenner why is it a success not rejecting 4xx or 5xx? Genuinely interested.

@stefanpenner
Copy link
Contributor Author

@stefanpenner why is it a success not rejecting 4xx or 5xx? Genuinely interested.

resolve !== success it means the code worked, 4xx and 5xx is not an exceptional case. Today we do use "exceptions" as control flow, but that historically has never worked out so well, maybe we should reconsider in this case?

@bcardarella
Copy link
Contributor

@stefanpenner that seems like a very systemic change for Ember's model hook itself. As-is how would ember-fetch work with the model hook and substates? Are we expected to manually throw in the result function to force the error substate?

@stefanpenner
Copy link
Contributor Author

Are we expected to manually throw in the result function to force the error substate?

Well, I would think our current approach is slightly difficient. Most systems understand NotFound and Error, ours currently does not. Maybe we can explore something to help address this?

We can also provide fetch/ajax that retains the existing semantics, which would be a drop-in.

Its also worth nothing that you wouldn't do (unless more conventions where baked into ember, which they could be).

model() {
  return fetch('something');
}

rather, it would be:

model() {
  return fetch('something').then((response) => response.json());
}

Which clearly starts to get verbose.

One interesting thought to improve the fidelity, without exploding the verbosity (and somewhat inspired by rails strong-params) may results in extensions to fetch, to enable something like?

model() {
  return fetch('something').expect(200).asJSON()
}

With maybe some sensible defaults if expect(200) is dropped, but asJSON() is called?

@bcardarella
Copy link
Contributor

@stefanpenner I would want a wider range of failure states than just NotFound and Error. I would want the entire 4xx - Client Error series and the entire 5xx Internal Server Error series. Basically, anything not in 200 - 299 should probably result in a failed state.

@stefanpenner
Copy link
Contributor Author

@bcardarella in all honestly, you actually want all the options. The strong convention in some cases, and in others you want full control.

I suspect fetch as the primtives goes a long way, we can then dress it up as i describe above (or similar)

@rwjblue
Copy link
Member

rwjblue commented Jul 1, 2015

incorrect, ember-fetch never falls back to native

This is not clearly a good thing to me. The primary reason to adopt fetch is that this will be a standard and we can someday remove our custom implementation. If that is not the goal, then what is?

@stefanpenner
Copy link
Contributor Author

If that is not the goal, then what is?

It can be the goal, unfortunately only realistic if a polyfil can be of sufficient fidelity to reasonably be a drop-in replacement. Unfortunately, I do not believe that to be the case yet. some context.

That being said, the polyfil should be more the sufficient for most use-cases, I believe GH itself uses it. Opting for the polyfil always does reduce 👣 🔫 's

Once it is deemed safe, we can remove this aspect of ember-fetch, patch pretender.

@bcardarella
Copy link
Contributor

Personally I'd like to see more real-world use cases of ember-fetch before we throw out ic-ajax as the default. I do think the fetch API is the future and the direction we should head in, but I also feel (based upon this thread) the implementation and impact to existing ember apps has not been fully thought through. If the intent of this RFC is to get fetch into ember-cli as the default in anticipation of the 1.0 landing sometime in the near future I don't feel confident that this will be as complete a solution as we should have.

I can start migrating the DockYard client apps over to ember-fetch and we can help develop use cases. It would be nice if some other companies/shops can do the same.

@stefanpenner
Copy link
Contributor Author

I can start migrating the DockYard client apps over to ember-fetch and we can help develop use cases. It would be nice if some other companies/shops can do the same.

Sounds good, please do.

Please note: I have been using it for some time, or I wouldn't have brought it up.

@cibernox
Copy link
Contributor

cibernox commented Jul 1, 2015

If mocking libraries will continue to work and we can provide a simplistic convenience ember-fetch/ajax that more or less mantains the semantics of ajax (reject on codes other than 2XX) I think it's more than enough for my use cases, and I like the idea of depend less and less on jquery.

jakecraige added a commit to thoughtbot/guides that referenced this issue Jul 1, 2015
Stefan is looking for people to try this out and give feedback.
We have enough Ember projects that we could contribute
good information to this RFC and the spec.

There isn't much risk to establishing this preference
because it's being polyfilled so it will work wherever.

The gain is that we will be able to
provide valuable feedback to the ember community.
Assuming this eventually get's merged we will also be ahead
of the curve and not need to update existing apps
to accomidate it.

> What is ember-fetch?
>
> * a wrapper around: https://github.com/github/fetch
> * which is a partial polyfil for  https://fetch.spec.whatwg.org/
> * more info: https://github.com/stefanpenner/ember-fetch
> * never falls back to native (so always works the same, and works with pretender)
>
> Why is this discussion happening now and not earlier?
>
> * ember 2 no longer supports IE8, and github/fetch is IE9+
> * fetch is a spec primitive of the DOM, it stands to reason we should prefer that over $.ajax
> * maybe we can catch issues to help adapt/improve/add to the spec
> * $.ajax is crazy quirky
> * uploads have progress in the request object (unsure if the polifill does this or not actually, need to verify)
>
> Cons:
>
> * its not $.ajax which, although not a spec thing, is basically a primitive many devs are used to
> * features like `$.ajaxPrefilter` are not obviously a thing in `fetch` (maybe some other approach ?)
> * .... ?
>
>
> Noop:
>
> * still no formal cancelation
>
> RFC: ember-cli/rfcs#19
@jakecraige
Copy link
Member

I'm proposing this to thoughtbot to use on our future ember apps here: thoughtbot/guides#342

@bf4
Copy link

bf4 commented Jul 2, 2015

So, @mitchlloyd introduced me to using import request from 'ic-ajax';, which has a nice promises impl request(url).then( (response) => { //stuff };, (reason) => { // stuff }; ). That's not too different from fetch, no? I'm testing this using ember-cli-mirage. https://github.com/pixelhandler/ember-fixturific/pull/1/files looked good but was a little involved. I bring up testing because I think it's important never got ic-ajax fixtures working.

@eccegordo
Copy link

Con: fetch resolves successfully on 4xx and 5xx errors, but $.ajax rejects.
this is both a CON and a PRO.

Think other HTTP libs, typically they don't throw an exception if the response was 500, rather they give you an object with the status code. Rather they only fail if the connection itself failed, or if some code exploded.

I think this is an astute point @stefanpenner is making here.

From the vantage point of the client a 5xx error is resolved. It is the server telling you something went wrong. You can't just reach across the boundary of HTTP and expect to change state on the backend when a problem happens. HTTP is a stateless protocol after all. So it seems perfectly logical to me that it resolves in this scenario.

Now the client has a few things it can do in this scenario:

  • interpret the error and throw (change the state locally)
  • swallow the error and handle failed response gracefully (or crash)
  • interpret the error, catch and retry, perhaps fixing the request to avoid the 5xx error next time.
  • give up because the error condition is truly unexpected

I am in favor of using fetch.

I can see why people might want $.ajax perhaps there is a way to trigger this behavior or make it conditionally available via addon. But using fetch as the out of the box standard seems sensible.

@jamesarosen
Copy link

give up because the error condition is truly unexpected

Without a richer API (like @stefanpenner's suggested .expect()), there are only two ways to do that:

// 1: exceptions as flow-control:
model() {
  return fetch('/something')
  .then(function(response) {
    if (response.status >= 400) { throw new Error("Whoops"); }
  });

// 2: wrap in a promise:
model() {
  return new Ember.RSVP.Promise(function(resolve, reject) {
    fetch('/something')
    .then(function(response) {
      if (response.status >= 400) { reject("Whoops"); }
    });
}

I agree that giving the app author more control to do things like retry or use backup data is good. But Ember.Route's hooks behave as they do and we should make sure we don't lose important ergonomics.

jakecraige added a commit to thoughtbot/guides that referenced this issue Aug 6, 2015
Stefan is looking for people to try this out and give feedback.
We have enough Ember projects that we could contribute
good information to this RFC and the spec.

There isn't much risk to establishing this preference
because it's being polyfilled so it will work wherever.

The gain is that we will be able to
provide valuable feedback to the ember community.
Assuming this eventually get's merged we will also be ahead
of the curve and not need to update existing apps
to accomidate it.

> What is ember-fetch?
>
> * a wrapper around: https://github.com/github/fetch
> * which is a partial polyfil for  https://fetch.spec.whatwg.org/
> * more info: https://github.com/stefanpenner/ember-fetch
> * never falls back to native (so always works the same, and works with pretender)
>
> Why is this discussion happening now and not earlier?
>
> * ember 2 no longer supports IE8, and github/fetch is IE9+
> * fetch is a spec primitive of the DOM, it stands to reason we should prefer that over $.ajax
> * maybe we can catch issues to help adapt/improve/add to the spec
> * $.ajax is crazy quirky
> * uploads have progress in the request object (unsure if the polifill does this or not actually, need to verify)
>
> Cons:
>
> * its not $.ajax which, although not a spec thing, is basically a primitive many devs are used to
> * features like `$.ajaxPrefilter` are not obviously a thing in `fetch` (maybe some other approach ?)
> * .... ?
>
>
> Noop:
>
> * still no formal cancelation
>
> RFC: ember-cli/rfcs#19
jakecraige added a commit to thoughtbot/guides that referenced this issue Aug 6, 2015
Stefan is looking for people to try this out and give feedback.
We have enough Ember projects that we could contribute
good information to this RFC and the spec.

There isn't much risk to establishing this preference
because it's being polyfilled so it will work wherever.

The gain is that we will be able to
provide valuable feedback to the ember community.
Assuming this eventually get's merged we will also be ahead
of the curve and not need to update existing apps
to accomidate it.

> What is ember-fetch?
>
> * a wrapper around: https://github.com/github/fetch
> * which is a partial polyfil for  https://fetch.spec.whatwg.org/
> * more info: https://github.com/stefanpenner/ember-fetch
> * never falls back to native (so always works the same, and works with pretender)
>
> Why is this discussion happening now and not earlier?
>
> * ember 2 no longer supports IE8, and github/fetch is IE9+
> * fetch is a spec primitive of the DOM, it stands to reason we should prefer that over $.ajax
> * maybe we can catch issues to help adapt/improve/add to the spec
> * $.ajax is crazy quirky
> * uploads have progress in the request object (unsure if the polifill does this or not actually, need to verify)
>
> Cons:
>
> * its not $.ajax which, although not a spec thing, is basically a primitive many devs are used to
> * features like `$.ajaxPrefilter` are not obviously a thing in `fetch` (maybe some other approach ?)
> * .... ?
>
>
> Noop:
>
> * still no formal cancelation
>
> RFC: ember-cli/rfcs#19
@taras
Copy link

taras commented Aug 11, 2015

I would like to propose that we consider using ember-ajax addon that I created as a transitionary step towards wider adoption of ember-fetch. ember-ajax is currently using Ember.$.ajax with code that was pulled out of Ember Data.

The addon already provides exception based error reporting and allows to customize what is considered a failed or successful response by overriding isSuccess method on the Ajax Service. The service doesn't currently provide any API equivalent to $.ajaxPrefilter, but we could experiment with an API that would provide similar functionality.

Using adapter pattern, we could make it possible for people to opt-in to using fetch to make requests instead of Ember.$.ajax. When we're sufficiently comfortable that the service provides all of the neccessary APIs, we could deprecate Ember.$.ajax completely.

Thoughts?

@jonathanKingston
Copy link
Member

@taras what are the advantages of the transnational change vs backporting the changes?

I'm struggling to see why not to make this transition; however real world demos would make this much easier too as mentioned.

@stefanpenner
Copy link
Contributor Author

resolution: ic-ajax -> ember-fetch is to big of a jump for default, although we would encourage users to use fetch, the transition is non-obvious.

Instead ic-ajax -> ember-ajax is the jump we are going to take.

jakecraige added a commit to thoughtbot/guides that referenced this issue Sep 8, 2015
Why:

* The ember-fetch RFC was an experiement that [failed].
* This is now the recommended approach from the ember-cli team.

[failed]: ember-cli/rfcs#19 (comment)

This change addresses the need by:

* Updating the copy and sample to prefer ember-ajax over ic-ajax
jakecraige added a commit to thoughtbot/guides that referenced this issue Sep 20, 2015
Why:

* The ember-fetch RFC was an experiement that [failed].
* This is now the recommended approach from the ember-cli team.

[failed]: ember-cli/rfcs#19 (comment)

This change addresses the need by:

* Updating the copy and sample to prefer ember-ajax over ic-ajax
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests