fix($resource): Delete $cancelRequest from toJSON() #15244

hereblur · 2016-10-11T03:57:26Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
result of ngResource.toJSON still include $cancelRequest

What is the new behavior (if this is a feature change)?
remove delete $cancelRequest from ngResource.toJSON's result.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

[ Y ] The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
[ N ] Tests for the changes have been added (for bug fixes / features)
[ N ] Docs have been added / updated (for bug fixes / features)

Other information:

gkalpak · 2016-10-11T21:50:24Z

Thx for the PR. This was an intentional omission though. Since $cancelRequest is always a function (and JSON.stringify() ignores functions), it shouldn't make any difference.

I am going to close this, but if you have a special usecase that breaks because of $cancelRequest(), please share below and I'll be happy to investigate.

hereblur · 2016-10-13T05:25:56Z

In my opinion, from the method name toJSON() we should be expected for a valid and ready to use of a JSON object.

In many cases, people need the literal object from server without extra keys.
I got wrong number when I do Object.keys( result ).length.

I think one should not have to do JSON.parse(JSON.stringifyr(result)) to get it works.

Thank you.

gkalpak · 2016-10-13T06:11:41Z

toJSON is a special method recognized by JSON.stringify() and is only intended to be used implicitly by that - i.e. not directly by the user (MDN docs).

That said, I guess it doesn't hurt to remove one more property ourselves (instead of relying on JSON.stringify()), so why not?

Reopening, but we need some tests for this (if we are making the change, we should make sure there will be no regressions in the future).

gkalpak · 2016-10-13T06:15:51Z

Hm...I just realized toJSON is publicly documented and the docs state that the returned object does not contain any of the extra properties. So, we should definitely fix that.

Thx @hereblur bringing it up.

Add test of pull request angular#15244 (angular#15244)

googlebot · 2016-10-13T07:43:00Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-10-13T07:43:00Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

hereblur · 2016-10-13T07:48:33Z

I signed it!

googlebot · 2016-10-13T07:48:34Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-10-13T07:48:34Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-10-13T08:01:13Z

CLAs look good, thanks!

googlebot · 2016-10-13T08:01:13Z

CLAs look good, thanks!

result.toJSON() still include $cancelRequest to the JSON output. just simply delete it.

Add test of pull request angular#15244 (angular#15244)

hereblur · 2016-10-13T08:05:53Z

@gkalpak Cool!, Thank you.
And I just pushed the test.

gkalpak · 2016-10-13T08:34:14Z

test/ngResource/resourceSpec.js

@@ -750,6 +750,7 @@ describe('basic usage', function() {
    var json = JSON.parse(angular.toJson(cc));
    expect(json.$promise).not.toBeDefined();
    expect(json.$resolved).not.toBeDefined();
+    expect(json.$cancelRequest).not.toBeDefined();


Can you also add an expectation that is was there before? (Similar to what we have for $promise/$resolved.)

I just realized that this test uses JSON.parse(angular.toJson(cc)), which means it would pass even without this PR.
Can you add a new test (similar to this one) that uses cc.toJSON() directly? (Leave this one as is - it doesn't hurt.)

@gkalpak

Improved test, per @gkalpak comment, add an expectation that is was there before.

gkalpak · 2016-10-13T12:06:41Z

test/ngResource/resourceSpec.js

@@ -750,6 +750,7 @@ describe('basic usage', function() {
    var json = JSON.parse(angular.toJson(cc));
    expect(json.$promise).not.toBeDefined();
    expect(json.$resolved).not.toBeDefined();
+    expect(json.$cancelRequest).not.toBeDefined();


I just realized that this test uses JSON.parse(angular.toJson(cc)), which means it would pass even without this PR.
Can you add a new test (similar to this one) that uses cc.toJSON() directly? (Leave this one as is - it doesn't hurt.)

create separated test since the existing test always pass without this PR.

gkalpak · 2016-10-20T15:32:38Z

test/ngResource/resourceSpec.js

@@ -737,7 +740,7 @@ describe('basic usage', function() {
    expect(person2).toEqual(jasmine.any(Person));
  });

-  it('should not include $promise and $resolved when resource is toJson\'ed', function() {
+  it('should not include $promise, $resolved when resource is toJson\'ed', function() {


Revert this change.

gkalpak · 2016-10-20T15:38:28Z

test/ngResource/resourceSpec.js

@@ -29,6 +29,9 @@ describe('basic usage', function() {
        }
      }

+    },
+    {
+      cancellable: true


Nit: It probably doesn't matter, but I would rather keep changes local, instead of affecting all tests. I.e. create a new, cancellable resource for your test.

create separated cancellable resource for test.

gkalpak · 2016-10-21T06:55:18Z

test/ngResource/resourceSpec.js

+
+    expect(creditCard.$cancelRequest).toBeDefined();
+
+    $httpBackend.flush();


Is this necessary?

Yes, we need to make sure It was there before remove it. same as the test next to it (for $promise and $resolved).

It should be there even before flush $http, isn't it?
If $httpBackend doesn't complain, just remove the flushing.

Closes #15244

Closes angular#15244

gkalpak closed this Oct 11, 2016

gkalpak added the resolution: works as expected label Oct 11, 2016

gkalpak reopened this Oct 13, 2016

gkalpak added type: bug component: ngResource frequency: low needs: test severity: confusing and removed resolution: works as expected labels Oct 13, 2016

gkalpak assigned gkalpak and unassigned gkalpak Oct 13, 2016

petebacondarwin added this to the 1.6.x (post 1.6.0) milestone Oct 13, 2016

hereblur pushed a commit to hereblur/angular.js that referenced this pull request Oct 13, 2016

test($resource): Delete $cancelRequest from toJSON()

a31c088

Add test of pull request angular#15244 (angular#15244)

googlebot added the cla: no label Oct 13, 2016

hereblur force-pushed the master branch from a31c088 to d60bbc7 Compare October 13, 2016 08:01

googlebot added cla: yes and removed cla: no labels Oct 13, 2016

fix($resource): Delete $cancelRequest from toJSON()

0bd9b2e

result.toJSON() still include $cancelRequest to the JSON output. just simply delete it.

test($resource): Delete $cancelRequest from toJSON()

d60bbc7

Add test of pull request angular#15244 (angular#15244)

gkalpak suggested changes Oct 13, 2016

View reviewed changes

test($resource): Delete $cancelRequest from toJSON()

4ce6272

Improved test, per @gkalpak comment, add an expectation that is was there before.

gkalpak suggested changes Oct 13, 2016

View reviewed changes

test($resource): Delete $cancelRequest from toJSON()

dc53f48

create separated test since the existing test always pass without this PR.

gkalpak suggested changes Oct 20, 2016

View reviewed changes

test($resource): Delete $cancelRequest from toJSON()

4761592

create separated cancellable resource for test.

gkalpak reviewed Oct 21, 2016

View reviewed changes

gkalpak pushed a commit that referenced this pull request Dec 29, 2016

fix($resource): delete $cancelRequest() in toJSON()

086c5d0

Closes #15244

gkalpak closed this in 922a83e Dec 29, 2016

gkalpak pushed a commit that referenced this pull request Dec 29, 2016

fix($resource): delete $cancelRequest() in toJSON()

4f3858e

Closes #15244

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

fix($resource): delete $cancelRequest() in toJSON()

126d68f

Closes angular#15244

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($resource): Delete $cancelRequest from toJSON() #15244

fix($resource): Delete $cancelRequest from toJSON() #15244

hereblur commented Oct 11, 2016

gkalpak commented Oct 11, 2016 •

edited

Loading

hereblur commented Oct 13, 2016

gkalpak commented Oct 13, 2016

gkalpak commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

hereblur commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

hereblur commented Oct 13, 2016

gkalpak Oct 13, 2016

hereblur Oct 13, 2016

gkalpak Oct 13, 2016

gkalpak Oct 13, 2016

gkalpak Oct 20, 2016

gkalpak Oct 20, 2016

gkalpak Oct 21, 2016

hereblur Oct 21, 2016

gkalpak Oct 21, 2016


		expect(creditCard.$cancelRequest).toBeDefined();

		$httpBackend.flush();

fix($resource): Delete $cancelRequest from toJSON() #15244

fix($resource): Delete $cancelRequest from toJSON() #15244

Conversation

hereblur commented Oct 11, 2016

gkalpak commented Oct 11, 2016 • edited Loading

hereblur commented Oct 13, 2016

gkalpak commented Oct 13, 2016

gkalpak commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

hereblur commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

googlebot commented Oct 13, 2016

hereblur commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 11, 2016 •

edited

Loading