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

Fixing possible bug in initialization of value #4545

Closed
wants to merge 1 commit into from
Closed

Fixing possible bug in initialization of value #4545

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 20, 2013

Hello, I think I found a bug in initialisation of value value in angular-resource.js.

I tried to fetch a Json array from my Api server and was getting the following error:

TypeError: Object #<Resource> has no method 'push'

I compared code from v1.0.6 (which is working okay) with the code from v1.2.0-rc.3 and found the following difference:

v1.0.6:
var value = this instanceof Resource ? this : (action.isArray ? [] : new Resource(data));

v1.2.0-rc.3:
var isInstanceCall = data instanceof Resource;
var value = isInstanceCall ? data : (action.isArray ? [] : new Resource(data));

And indeed, data is the second parameter to the Resource[name] = function(a1, a2, a3, a4) which is called from Resource.prototype['$' + name] = function(params, success, error) as:

var result = Resource[name](params, this, success, error);

So this in the scope of Resource.prototype will, presumably, be always instance of Resource and var isInstanceCall = data instanceof Resource; will be always set to true.

I am, most likely, wrong, but the following change fixed my problem. I think it is worth for someone from the team to take a look at what is going in there.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@petebacondarwin
Copy link
Contributor

@sanyax7 - Can you provide a unit test that demonstrates this problem that you are fixing? Also can you ensure that you have signed the CLA?
Thanks

@smashercosmo
Copy link

I made a plunker that illustrates this regression.
http://plnkr.co/edit/Eply3y7h3xDjTInZY0SI?p=preview

@ghost ghost assigned tbosch Nov 20, 2013
@tbosch
Copy link
Contributor

tbosch commented Nov 21, 2013

Hello @smashercosmo, I looked at your example.

It shows that calling DataResource.save(item) does not return a new DataResrouce object (but a promise instead). This is the same behavior as calling item.$save(). However, in our docs (http://docs.angularjs.org/api/ngResource.$resource) there is the sentence: "Class actions return empty instance (with additional properties below). Instance actions return promise of the action."

So you would expect calling DataResource.save(item) to return a new DataResource, and calling item.$save() to return a promise, right?

@tbosch
Copy link
Contributor

tbosch commented Nov 21, 2013

I looked at your PR also. This is the first step, but we also need to set this to the correct value when calling Resource[name] from a prototype method. Se we need

  1. var isInstanceCall = this instanceof Resource; instead of var instanceCall = data instanceof Resource in https://github.com/angular/angular.js/blob/master/src/ngResource/resource.js#L442
  2. var result = Resource[name].call(this, params, this, success, error); instead of var result = Resource[name](params, this, success, error); in https://github.com/angular/angular.js/blob/master/src/ngResource/resource.js#L525:

This change was introduced by this commit: 05772e1

@tbosch
Copy link
Contributor

tbosch commented Nov 21, 2013

I created a pull request for this that also includes a test.

Thanks for filing this PR!

@smashercosmo
Copy link

@tbosch

So you would expect calling DataResource.save(item) to return a new DataResource, and calling item.$save() to > > return a promise, right?

yes, this is the exact behaviour, that I expect

P.S. Thanks for proper PR

tbosch added a commit to tbosch/angular.js that referenced this pull request Nov 21, 2013
…methods on resources.

Previously, calling `MyResource.save(myResourceInstance)`returned
a promise, in contrast to the docs for `$resource`. However,
calling `MyResource.save({name: 'Tobias"})`already correctly
returned a resource instance.

Fixes angular#4545.
@tbosch tbosch closed this in f6ecf9a Nov 21, 2013
@ghost
Copy link
Author

ghost commented Nov 25, 2013

Thank you for fixing this :)

@ghost ghost deleted the resource_value_init_fix branch November 25, 2013 13:06
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…methods on resources.

Previously, calling `MyResource.save(myResourceInstance)`returned
a promise, in contrast to the docs for `$resource`. However,
calling `MyResource.save({name: 'Tobias"})`already correctly
returned a resource instance.

Fixes angular#4545.
Closes angular#5061.
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…methods on resources.

Previously, calling `MyResource.save(myResourceInstance)`returned
a promise, in contrast to the docs for `$resource`. However,
calling `MyResource.save({name: 'Tobias"})`already correctly
returned a resource instance.

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

Successfully merging this pull request may close these issues.

4 participants