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

Conversation

kwypchlo
Copy link
Member

What

Fix for issue #656

Why

I did not take into account such basic scenario - no test case was failing and it all seemed to work on my app

How

field object does not contain model property that is the model itself for the basic field usage where the model is $scope.model. I assumed we could then assign this $scope.model as a property to field object and operate only on this property. This would make it consistent with the scenarios when field.model was passed as a string and resolved to an object (with a watcher) or assigned as an custom object.
This was a mistake because when scope.model reference changed, field.model referenced old model object. To make it work I would have to put a watcher on scope.model and update field.model accordingly but this would add a lot of unnecessary watchers so I've reverted to the original solution where we use construction field.model || scope.model which works fine now.

One test case added, one test case fixed.

For issue #656

Checklist:

closes #656

@codecov-io
Copy link

Current coverage is 96.03%

Merging #657 into master will decrease coverage by -0.01% as of 84d5d97

@@            master    #657   diff @@
======================================
  Files           16      16       
  Stmts         1163    1161     -2
  Branches         0       0       
  Methods          0       0       
======================================
- Hit           1117    1115     -2
  Partial          0       0       
  Missed          46      46       

Review entire Coverage Diff as of 84d5d97


Uncovered Suggestions

  1. +0.35% via ...alidationMessages.js#25...28
  2. +0.26% via src/test.utils.js#38...40
  3. +0.26% via ...s/formlyUsability.js#18...20
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@@ -509,8 +526,9 @@ describe('formly-form', () => {
}

scope.$digest()
$timeout.flush()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can scope.$digest() be removed after adding $timeout.flush()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, $digest is needed to trigger the watcher on formly directive for the model property and $timeout is just a dirty workaround for runExpressions method :)

@BarryThePenguin
Copy link
Contributor

Ok, looks good

BarryThePenguin added a commit that referenced this pull request Mar 10, 2016
fix(model): keep track of correct model reference
@BarryThePenguin BarryThePenguin merged commit 6365ce3 into formly-js:master Mar 10, 2016
@kwypchlo kwypchlo deleted the feature/fix-model-reference-update branch March 10, 2016 12:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8.0 + Model Changes not updating form
3 participants