Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Pr1701 #2130

Closed
wants to merge 2 commits into from
Closed

Pr1701 #2130

wants to merge 2 commits into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Mar 9, 2013

No description provided.

@mhevery mhevery mentioned this pull request Mar 9, 2013
Closed
@inukshuk
Copy link
Contributor

inukshuk commented Mar 9, 2013

Looks awesome, thanks!

theGeekPirate
theGeekPirate reviewed Mar 9, 2013
View reviewed changes
src/ng/http.js
* });
* </pre>
*
* # Response interceptors (DEPRECATED)
*
* Before you start creating interceptors, be sure to understand the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove these two comments since they were added above

@IgorMinar
Copy link
Contributor

The PR looks almost good, the biggest problems I see are:

  • the execution order of response interceptors should be reversed
  • the extra $digest()s that are required even when interceptors are not being used will break existing tests
  • requestReject and responseReject should be renamed to requestError and responseError

@inukshuk @mhevery
feat(http): support request/response promise chaining
e144913 
myApp.factory('myAroundInterceptor', function($rootScope, $timeout) {
    return function(configPromise, responsePromise) {
        return {
            request: configPromise.then(function(config) {
                return config
            });
            response: responsePromise.then(function(response) {
                return 'ha!';
            }
        });
}

myApp.config(function($httpProvider){
    $httpProvider.aroundInterceptors.push('myAroundInterceptor');
});
@IgorMinar
Copy link
Contributor

LGTM. can you tests this on DFA before merging? We should understand the consequences of requiring the extra digest in tests...

@IgorMinar IgorMinar mentioned this pull request Mar 12, 2013
Closed
@al-the-x
Copy link
Contributor

Awesome, guys. This is an excellent improvement over @inukshuk's initial work, and I'm glad to see him contributing to this PR as well. Good work!

@inukshuk
Copy link
Contributor

To give credit where credit is due, this is all @mhevery and @IgorMinar here – there are just some leftovers from the previous commit : )

@ghost ghost assigned mhevery Mar 13, 2013
@al-the-x
Copy link
Contributor

Ah, don't destroy my dreams of getting on the contributors list one day, @inukshuk...! ;)

@djsmith42
Copy link

Impressive work, guys. Clever use of promise chaining to wrap the server request. Can't wait to start using this.

@marcospassos
Copy link

I'm looking forward for this release.

@djsmith42
Copy link

Any word on this PR's readiness for merging?

@leifhanack
Copy link

Cool. Any release date known for this feature?

@gesellix
Copy link
Contributor

Will this patch be included in the upcoming 1.2.0 release?

@jbdeboer
Copy link
Contributor

There is a bug in this PR where the error callback of injected responseInterceptors were not getting called. I have a fix in https://github.com/jbdeboer/angular.js/commits/pr-2130-fix

Other than that everything appears to be happy and we should be able to land this PR very soon.

@jbdeboer
Copy link
Contributor

Landed in 4ae4681

@jbdeboer jbdeboer closed this Mar 27, 2013
@djsmith42
Copy link

Yes!! Great work. Can't wait to upgrade!

@aflock
Copy link

aflock commented May 16, 2013

Looking at this, I don't understand the decision to require the extra $digests, both for tests and for functionality.
Were @IgorMinar 's concerns ever resolved?
It's manifested in alot of unintended behavior during the upgrade from 1.1.3->1.1.4 (see #2438, #2431, #2557, #2371), and I'm very curious about the motivations here.

@c0bra was able to fix this by immediately triggering a new tick (see https://gist.github.com/c0bra/5594851)

@aflock aflock mentioned this pull request May 16, 2013
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@mhevery mhevery

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.