fix($parse): create undefined arrays in assigements #9850

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@pkozlowski-opensource

Fixes #9820

@caitp caitp commented on the diff Oct 30, 2014
test/ng/parseSpec.js
@@ -1164,6 +1164,23 @@ describe('parser', function() {
fn.assign(scope, 123);
expect(scope.a.b.c).toEqual(123);
}));
+
+ it('should create undefined objects in assignment expressions', inject(function($parse) {
@caitp
caitp Oct 30, 2014 Contributor

don't we already have a test for this?

@pkozlowski-opensource
pkozlowski-opensource Oct 30, 2014 Member

surpassingly I can see only very limited number of tests for the assign:
https://github.com/angular/angular.js/blob/2a0254e1816b64c1235e3d71af1eefe5466d1fe3/test/ng/parseSpec.js#L1143

But maybe I'm not looking at the right place...

@caitp
caitp Oct 30, 2014 Contributor

I didn't check, I'd just be surprised if we didn't. More tests are always good though

@caitp
Contributor
caitp commented Oct 30, 2014

I'm not sure how I feel about this --- I guess it's okay maybe, but I dunno. the patch looks fine though

@mary-poppins mary-poppins added cla: yes and removed cla: no labels Oct 30, 2014
@pkozlowski-opensource

Yeh, honestly I also don't know what to think about this one :-) I guess it can be useful in some cases but I'm just afraid that we are introducing some corner cases I can't see atm.

@lgalfaso
Member

I am torn about this, the reason being that if you have

<input ng-model="model[0]"/>
<input ng-model="model.foo"/>

then depending on the order the user fills the fields, we end up with an array or an object.

With this in mind, I lean towards not merging this, but not strong enought to oppose it getting merge

@lgalfaso
Member

Ah, I see that @pkozlowski-opensource already posted the same issue in the original report. Sorry for the noise

@pkozlowski-opensource

@lgalfaso yeh, this was exactly my first worry as well.

Anyway, I don't feel strongly about this one either way. Let's either merge this or close as "won't fix" - there is no point spending too much time on this one.

@caitp
Contributor
caitp commented Oct 30, 2014

I kinda feel like we should shy away from assignment in expressions and get people to initialize their data --- is that too unrealistic? :(

@zgmnkv
Contributor
zgmnkv commented Oct 31, 2014

Please look at my comment in #9820
Regarding manual initialization: it's not always easy, for example with nested arrays like model[0].prop.arrayProp[1].innerProp. We need to watch model changes to initialize arrayProp.

@lgalfaso
Member

This issue was discussed and the agreement was to make this PR a wont fix

@lgalfaso lgalfaso closed this Nov 11, 2014
@pjlnmix pjlnmix referenced this pull request in formly-js/angular-formly Aug 18, 2016
Closed

Key with array index saves as object unless array already exists #706

@pjlnmix pjlnmix added a commit to pjlnmix/angular-formly that referenced this pull request Aug 19, 2016
@pjlnmix pjlnmix feat(formly-field): Adds parser for keys that contain arrays
angular. does not (and will not angular/angular.js#9850) properly
handle arrays in keys unless they have already been created in the model. This fix/feature adds a
separate parser for these circumstances and a formlyConfig.extras flag to control its use. The flag
is in place to minimize impact on current functionality of the setter.

This is in support of formly-js#706
7f8601c
@pjlnmix pjlnmix referenced this pull request in formly-js/angular-formly Aug 19, 2016
Merged

feat(formly-field): Adds parser for keys that contain arrays #709

3 of 3 tasks complete
@kentcdodds kentcdodds added a commit to formly-js/angular-formly that referenced this pull request Sep 6, 2016
@pjlnmix @kentcdodds pjlnmix + kentcdodds feat(formly-field): Adds parser for keys that contain arrays (#709)
angular. does not (and will not angular/angular.js#9850) properly
handle arrays in keys unless they have already been created in the model. This fix/feature adds a
separate parser for these circumstances and a formlyConfig.extras flag to control its use. The flag
is in place to minimize impact on current functionality of the setter.

This is in support of #706
fc45fb3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment