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

fix(shallowCopy) logic error where '$foo' would be ignored #6033

Closed
wants to merge 2 commits into from

Conversation

basarat
Copy link
Contributor

@basarat basarat commented Jan 29, 2014

Introduced in this commit : cb29632

@caitp
Copy link
Contributor

caitp commented Jan 31, 2014

Hey, so going back to at least 1.2.3, we weren't ignoring properties with a single dollar-sign prefix (and, there are some potential uses for properties with these names, like writing queries to be fed directly into mongodb, not that I recommend anyone do that).

This would be a breaking change, although I'm not totally sure about the impact.

@basarat
Copy link
Contributor Author

basarat commented Jan 31, 2014

so going back to at least 1.2.3, we weren't ignoring properties with a single dollar-sign prefix

shallowCopy should only ignore $$anything not $anything. Before 1.2.3 it was $$anything only. Then the change was made to new code for performance which changed it to ignore $anything by mistake. If we only wanted $anything then key.charAt(0) != '$' would have been sufficient which is not the original code.

@caitp
Copy link
Contributor

caitp commented Jan 31, 2014

Ah, I see what you're saying, just tested on a fiddle. Okay, so this is a proper fix and not a breaking change.

What I'd like to see before LGTM-ing this is some tests added so that we can guard against breaking this inadvertently in the future.

@IgorMinar I'd like to get this into 1.2.11 once tests are added, does that look alright to you?

@petebacondarwin
Copy link
Contributor

LGTM. We should have unit tests to cover this

@caitp
Copy link
Contributor

caitp commented Feb 1, 2014

I'm not sure if they've already begun cutting the release or not... this might not be able to make it in this week :(

@caitp
Copy link
Contributor

caitp commented Feb 1, 2014

I just realized that this is actually a dupe of #5666. Hm. First one to add tests gets merged? :D

@tbosch tbosch modified the milestones: 1.2.12, 1.2.11 Feb 3, 2014
@atomrc
Copy link
Contributor

atomrc commented Feb 4, 2014

@caitp Just updated my pull request with tests for shallowCopy in Angular.js and shallowCopyAndClear in ngResource

#5666

Am I to be merged ;) ?

caitp pushed a commit to caitp/angular.js that referenced this pull request Feb 4, 2014
… requests/responses

ngResource no longer filters properties prefixed with a single "$" character from requests or
responses, correcting a regression introduced in 1.2.6 (cb29632).

Closes angular#5666
Closes angular#6080
Closes angular#6033
caitp pushed a commit to caitp/angular.js that referenced this pull request Feb 4, 2014
… requests/responses

ngResource no longer filters properties prefixed with a single "$" character from requests or
responses, correcting a regression introduced in 1.2.6 (cb29632).

Closes angular#5666
Closes angular#6080
Closes angular#6033
@caitp caitp closed this in d2e4e49 Feb 4, 2014
@caitp
Copy link
Contributor

caitp commented Feb 4, 2014

@thomasbelin4 yes thank you, merged in d2e4e49

@atomrc
Copy link
Contributor

atomrc commented Feb 4, 2014

Hell Yeah :) Thanks @caitp

@basarat basarat deleted the patch-6 branch February 5, 2014 06:31
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.

5 participants