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

fix($http): do not modify the config object passed into $http short methods #11764

Closed
wants to merge 1 commit into from
Closed

fix($http): do not modify the config object passed into $http short methods #11764

wants to merge 1 commit into from

Conversation

maxthyen
Copy link
Contributor

Update $http's createShortMethods and createShortMethodsWithData to extend an empty object instead of the passed-in config.

Previously, since $http was extending the passed-in config, the changes to the config object persisted even after the call to $http's get/post/etc. returned:

var config = { some_key: 1 };
$http.get('/testUrl', config);
// config now contains { some_key: 1, url: '/testUrl', method: 'GET' }

This causes unexpected behavior if that config object is reused in subsequent calls to $http. The existing test in httpSpec was not properly testing this situation.

@johnculviner
Copy link

👍 this caused a pretty lengthy head scratching session for @maxthyen and I until the behavior was realized. Can't think of any reasons why this would be desirable?

@matthewdordal
Copy link

👍 being able to reuse a config object without it being changed under the hood is ideal.

…ethods

Update $http's createShortMethods and createShortMethodsWithData
to extend an empty object instead of the passed-in config.
Previously, since $http was extending the passed-in config,
the changes to the config object persisted even after the call to $http's
get/post/etc. returned. This causes unexpected behavior if that
config object is reused in subsequent calls to $http.
The existing test in httpSpec was not properly testing this situation.
@maxthyen maxthyen changed the title feat($http): do not modify the config object passed into $http short methods fix($http): do not modify the config object passed into $http short methods May 3, 2015
@lgalfaso
Copy link
Contributor

I like this PR, just wondering if anybody is relying on this odd behavior to think that this is a breaking change.
@petebacondarwin WDYT?

@petebacondarwin
Copy link
Contributor

I agree. It looks good. We should merge it now and label it as a small breaking change - just in case.

@petebacondarwin
Copy link
Contributor

Do you want to take that @lgalfaso ?

lgalfaso pushed a commit to lgalfaso/angular.js that referenced this pull request May 18, 2015
…ethods

Update $http's createShortMethods and createShortMethodsWithData
to extend an empty object instead of the passed-in config.
Previously, since $http was extending the passed-in config,
the changes to the config object persisted even after the call to $http's
get/post/etc. returned. This causes unexpected behavior if that
config object is reused in subsequent calls to $http.
The existing test in httpSpec was not properly testing this situation.

Closes: angular#11764
@lgalfaso
Copy link
Contributor

landed as f7a4b48

@lgalfaso lgalfaso closed this May 18, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
…ethods

Update $http's createShortMethods and createShortMethodsWithData
to extend an empty object instead of the passed-in config.
Previously, since $http was extending the passed-in config,
the changes to the config object persisted even after the call to $http's
get/post/etc. returned. This causes unexpected behavior if that
config object is reused in subsequent calls to $http.
The existing test in httpSpec was not properly testing this situation.

Closes: angular#11764
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants