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

fix(copy) $resource throws TypeError when json has both properties & arrays #2255

Closed
wants to merge 1 commit into from
Closed

fix(copy) $resource throws TypeError when json has both properties & arrays #2255

wants to merge 1 commit into from

Conversation

joelwreed
Copy link

Closes #1044

@petebacondarwin
Copy link
Contributor

Hi @joelwreed - it looks like there is a lot of interest in this patch. Right now, though your unit test doesn't fail even if you don't add the patch.

  • Can you provide a unit test that fails before you apply the fix?

@petebacondarwin
Copy link
Contributor

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
  • PR doesn't introduce new api
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • [ ] PR contains e2e tests (if suitable)
  • [ ] PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@petebacondarwin
Copy link
Contributor

Once you have a failing unit test can you finesse the commit message. See the guidelines above. First line is a bit long and should describe what is being changed not what is the problem.

@jbdeboer
Copy link
Contributor

Checked the list and I don't see @joelwreed's signed CLA. Joel, if you've signed under a different name or as a company, let us know!

@penfold
Copy link

penfold commented Jun 15, 2013

Can this fix be merged in?

@petebacondarwin
Copy link
Contributor

I don't think this is actually a real bug, but an incorrect configuration of $resource. See my comments on the issue: #1044 (comment)

I think we should have a new PR that provides a better error message in $resource when the response doesn't match what is configured.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jun 25, 2013
@petebacondarwin
fix(resource): check whether response matches action.isArray
4c5574b 
When using $resource you must setup your actions carefully based on what the server returns. If the server responds to a request with an array then you must configure the action with `isArray:true` and vice versa.  The built-in `get` action defaults to `isArray:false` and the `query` action defaults to `isArray:true`, which is must be changed if the server does not do this.
Before the error message was an exception inside angular.copy, which didn't explain what the real problem was.
Rather than changing the way that angular.copy works, this change ensures that a better error message is provided to the programmer if they do not set up their resource actions correctly.

Closes angular#2255, angular#1044
@petebacondarwin petebacondarwin mentioned this pull request Jun 25, 2013
Closed
@petebacondarwin
Copy link
Contributor

Please take a look at #3054 as an alternative.

ksheedlo pushed a commit to ksheedlo/angular.js that referenced this pull request Jul 19, 2013
@petebacondarwin @ksheedlo
fix(resource): check whether response matches action.isArray
053a0da 
When using $resource you must setup your actions carefully based on what the server returns. If the server responds to a request with an array then you must configure the action with `isArray:true` and vice versa.  The built-in `get` action defaults to `isArray:false` and the `query` action defaults to `isArray:true`, which is must be changed if the server does not do this.
Before the error message was an exception inside angular.copy, which didn't explain what the real problem was.
Rather than changing the way that angular.copy works, this change ensures that a better error message is provided to the programmer if they do not set up their resource actions correctly.

Closes angular#2255, angular#1044
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jul 30, 2013
@petebacondarwin
fix(resource): check whether response matches action.isArray
544ce8c 
When using $resource you must setup your actions carefully based on what the server returns.
If the server responds to a request with an array then you must configure the action with
`isArray:true` and vice versa.  The built-in `get` action defaults to `isArray:false` and the
`query` action defaults to `isArray:true`, which is must be changed if the server does not do this.
Before the error message was an exception inside angular.copy, which didn't explain what the
real problem was. Rather than changing the way that angular.copy works, this change ensures that
a better error message is provided to the programmer if they do not set up their resource actions
correctly.

Closes angular#2255, angular#1044
petebacondarwin added a commit that referenced this pull request Jul 31, 2013
@petebacondarwin
fix(resource): check whether response matches action.isArray
a644ca7 
When using $resource you must setup your actions carefully based on what the server returns.
If the server responds to a request with an array then you must configure the action with
`isArray:true` and vice versa.  The built-in `get` action defaults to `isArray:false` and the
`query` action defaults to `isArray:true`, which is must be changed if the server does not do this.
Before the error message was an exception inside angular.copy, which didn't explain what the
real problem was. Rather than changing the way that angular.copy works, this change ensures that
a better error message is provided to the programmer if they do not set up their resource actions
correctly.

Closes #2255, #1044
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

$resource throws TypeError when json has both properties and arrays
4 participants
@joelwreed @petebacondarwin @jbdeboer @penfold