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

fix(ngResource): don't convert literal values into Resource objects when isArray is true #7741

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Jun 7, 2014

Previously non-object literals would be thrown out of Resource responses with isArray===true, or otherwise converted into Objects (in the case of string literals). The reason for this is because shallowClearAndCopy iterates over keys, and copies keys into the destination. Iterating over String keys results in integer keys, with a single-character value.

Not converting non-objects to Resources means that you lose the ability to perform Resource operations on them. However, they become usable as strings, numbers, or booleans, which is important.

In the future, it would be useful to make these useful as Resources while still retaining their primitive value usefulness. (@rodyhaddad can you think of a way to do this? You're the master of ES5 valueOf, as far as I know :P)

Closes #6314

…hen isArray is true

Previously non-object literals would be thrown out of Resource responses with isArray===true, or
otherwise converted into Objects (in the case of string literals). The reason for this is because
shallowClearAndCopy iterates over keys, and copies keys into the destination. Iterating over String
keys results in integer keys, with a single-character value.

Not converting non-objects to Resources means that you lose the ability to perform Resource operations
on them. However, they become usable as strings, numbers, or booleans, which is important.

In the future, it would be useful to make these useful as Resources while still retaining their primitive
value usefulness.

Closes angular#6314
@caitp caitp added this to the 1.3.0-beta.12 milestone Jun 7, 2014
@caitp caitp added cla: yes and removed cla: no labels Jun 7, 2014
@rodyhaddad
Copy link
Contributor

You can overwrite the valueOf of a given Resource instance, and make it return the primitive value. That would make the object take its original value when used with operators that don't work with objects (e.g. +).

But I believe it would create bugs that are hard to track for our users ("an empty Resource instance that sometimes behave like a primitive and has no data on it 😯 ") :

Maybe Resources can have hooks to handle non-object responses?


+1 for the issue this PR tries to solve though :-)

@caitp
Copy link
Contributor Author

caitp commented Jun 9, 2014

I don't think we can pretend Resource objects are primitives, since it wouldn't work when writing to them and stuff. It's probably adequate to just not convert them to Resources unless they're objects, since you probably aren't going to try to send off the primitives on their own.

But, someone else can think about that

@rodyhaddad
Copy link
Contributor

We're using typeof item === "object" instead of isObject(item)

What if it the array is [null]?

With this PR, the resulting array contains an empty Resource object.
Do we want that? or do we want the resulting array to contain null too (breaking change)

@caitp
Copy link
Contributor Author

caitp commented Jun 13, 2014

its not a breaking change, it is already empty/not copying anything in (for key in null) { ... }). probably better not to get rid of it though

@rodyhaddad
Copy link
Contributor

Currently if the response is:
['a', null],
it results in:
['a', new Ressource(null)] // empty resource (no id, so thing like $save act weird?)
This PR doesn't change that

I just want to be sure that it's the behavior we want. Maybe we want it to result in:
['a', null]
which would be a breaking change that's worth noting

@IgorMinar
Copy link
Contributor

Not wrapping null into a resource instance sounds reasonable. We should add a test for it. Otherwise lgtm.

rodyhaddad pushed a commit to rodyhaddad/angular.js that referenced this pull request Jun 13, 2014
…hen isArray is true

Previously non-object literals would be thrown out of Resource responses with isArray===true, or
otherwise converted into Objects (in the case of string literals). The reason for this is because
shallowClearAndCopy iterates over keys, and copies keys into the destination. Iterating over String
keys results in integer keys, with a single-character value.

Not converting non-objects to Resources means that you lose the ability to perform Resource operations
on them. However, they become usable as strings, numbers, or booleans, which is important.

In the future, it would be useful to make these useful as Resources while still retaining their primitive
value usefulness.

Closes angular#6314
Closes angular#7741
@caitp caitp closed this in 16dfcb6 Jun 13, 2014
rodyhaddad pushed a commit to rodyhaddad/angular.js that referenced this pull request Jun 13, 2014
…hen isArray is true

Previously non-object literals would be thrown out of Resource responses with isArray===true, or
otherwise converted into Objects (in the case of string literals). The reason for this is because
shallowClearAndCopy iterates over keys, and copies keys into the destination. Iterating over String
keys results in integer keys, with a single-character value.

Not converting non-objects to Resources means that you lose the ability to perform Resource operations
on them. However, they become usable as strings, numbers, or booleans, which is important.

In the future, it would be useful to make these useful as Resources while still retaining their primitive
value usefulness.

Closes angular#6314
Closes angular#7741
rodyhaddad pushed a commit to rodyhaddad/angular.js that referenced this pull request Jun 13, 2014
…hen isArray is true

Previously non-object literals would be thrown out of Resource responses with isArray===true, or
otherwise converted into Objects (in the case of string literals). The reason for this is because
shallowClearAndCopy iterates over keys, and copies keys into the destination. Iterating over String
keys results in integer keys, with a single-character value.

Not converting non-objects to Resources means that you lose the ability to perform Resource operations
on them. However, they become usable as strings, numbers, or booleans, which is important.

In the future, it would be useful to make these useful as Resources while still retaining their primitive
value usefulness.

Closes angular#6314
Closes angular#7741
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.

$resource.query turn strings into objects in response array
3 participants